-
-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat: embed vault views into notes #369
base: dev
Are you sure you want to change the base?
Conversation
Overall, everything looks good. I made sure to test it out. There are a few small things like padding/scrolling that need to be fixed when the view is embedded, but this is a great MVP for this feature. Thanks for upgrading the Typescript version to 5.6.x. That actually solves an error that I was getting on build because of the I added a few comments on things that I think could be improvement. Thanks for your contribution and diving into Svelte! |
// Embedded settings | ||
export let embedSettings: Record<string, any> = {}; | ||
|
||
let embedded = embedSettings?.embedded ?? false; | ||
let embeddedGroup = embedSettings?.group ?? null; | ||
let embeddedView = embedSettings?.view ?? null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you make an interface for the expected settings instead of using Record<string,any>
For example,
interface EmbedSettings {
group?: string;
view: string;
}
and then set the embedSettings to that use that interface undefined by default?
export let embedSettings: EmbedSettings | undefined = undefined;
// Note: Multiple groups can be the same name, in which case the first group found will be selected | ||
function selectGroupByName(name: string) { | ||
const { groups } = customFilter; | ||
const group = groups.find((group) => group.name === name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to make the group case insensitive.
group.name.toLowercase() === name.toLowercase()
So someone could put
```vault-explorer
group: reading
```
Open to thoughts on this
@container grid-container (min-width: 500px) { | ||
.vault-explorer-grid-card { | ||
max-width: min(50% - 1rem, 425px); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is @container used in this case? Is it used to place this style inside of the grid-card.svelte
file instead of in the parent component?
@JohnGemstone I am sorry, I thought I had finished the review. |
This feature allows users to access Vault-Explorer views directly within their notes, streamlining the process of navigating between views without needing to go to the side panel.
Users can define group views as usual and then reference them for embedding like so:
#368
Appreciate any feedback, questions or changes.
Cheers!