Sidebar #866
Sidebar #866
Conversation
|
|
Please add a screenshot and / or video recording of the result of what you did in this PR. |
There was a problem hiding this comment.
You may not need this class.
There was a problem hiding this comment.
I do think that every components in shadcn-svelte would be "primitives" na (in this case, "atoms"). To avoid confusion.
There was a problem hiding this comment.
I think SheetContent should be simpler na? Like in shadcn-svelte it's just a div. To decouple Sheet functionality to be composible.
There was a problem hiding this comment.
Every component in this PR is imported from shadcn-svelte though, I'm not sure how to apply your comments :(
| import { | ||
| SIDEBAR_COOKIE_MAX_AGE, | ||
| SIDEBAR_COOKIE_NAME, | ||
| SIDEBAR_WIDTH, | ||
| SIDEBAR_WIDTH_ICON | ||
| } from './constants.js'; |
There was a problem hiding this comment.
This logic should not be in pacakges/ui. Package should be something general. Hard coding or specific business logic should be passed from "app"
| onOpenChange(value); | ||
| // This sets the cookie to keep the sidebar state. | ||
| document.cookie = `${SIDEBAR_COOKIE_NAME}=${open}; path=/; max-age=${SIDEBAR_COOKIE_MAX_AGE}`; |
There was a problem hiding this comment.
Like previous comment. This should be from "app"
| //Please refer for to: https://www.shadcn-svelte.com/docs/components/sidebar | ||
| </script> | ||
|
|
||
| <!-- TODO: --> |
There was a problem hiding this comment.
Will you fix TODO in this PR?
| @@ -0,0 +1,101 @@ | |||
| <script lang="ts"> | |||
There was a problem hiding this comment.
This component should be in the app. Not package/ui
Package should be something simple / customizable. No complex UI
Why did you create this PR
What did you do
If anyone is using the sidebar refer to https://www.shadcn-svelte.com/docs/components/sidebar