-
Notifications
You must be signed in to change notification settings - Fork 42
Add sidebar component #177
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
base: main
Are you sure you want to change the base?
Conversation
|
Preview available at https://dioxuslabs.github.io/components/pr-preview/pr-177/ |
ealmloff
left a comment
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.
Thanks for working on this, the sidebar itself mostly looks right, but some of these changes will make the components harder to maintain in the long run.
Adding as everywhere is useful, but makes the attributes significantly harder to maintain. I would rather add support for some of this in core. An attributes macro that accepts the same syntax as the element body but expands to Vec<Attribut> would make this a lot easier to maintain:
attributes!{
width: 100,
onclick,
}This will also need some playright tests for the sidebar behavior before it can be merged
| pub side: Signal<SidebarSide>, | ||
| pub open: Signal<bool>, | ||
| pub set_open: Callback<bool>, | ||
| pub open_mobile: Signal<bool>, |
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.
Do we need two open signals/callbacks?
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.
Same interface with shadcn.
I thought they keep two open state management methods because of the separation between the sidebar (not mobile) and the sheet (mobile).
- diff UI pattern:
sidebarkeep displaying in layout no matters collapsible or not while mobile's display depending on open state - state persiste: I haven't complete the state persistence now, but in shadcn, sidebar's state persisted by document.cookie and sheet's state work as temporary
In conclusion, I think shadcn keeps two state managers for separation of concerns. Picking the combination of sheet and sidebar, this should be keep also
e0c3613 to
af5011d
Compare
9e0b4e7 to
673dd63
Compare
|
@ealmloff sidebar has done some optimization and added playwright test now. I keep the sheet/sidebar separation for the reason I replied. There is still a Clippy check errors about For the inscaleble |
|
It might make sense to upstream into dioxus itself at some point, but for now the macro is fine in the component library. As a slightly odd consequence of how we type check attributes, you still need to put the element name you expect to spread into |
related: #160
A new
sidebarcomponent is added with followed changes:r#asandmerged_attributein multiple componentsprimitive/lib.rs, addmeged_attributefor component withr#as, handle class/props of component(TODO: event handler)collapsible,tooltip,dropdown_menuseparatorgapin dropdown menu itemSidebarProviderand othersuse_sidebarhookuse_is_mobilehook (considering the right position in a shared module)playwright test)preview/src/main.rs,preview/src/mod.rsto allow block component display, by:- 2.1 extend
example!macro- 2.2 add Route and component for block component, implement isolate envrionment by
iframe(Only work in web platform now, notDesktop supported)- 2.3 refactoring
theme sync, addtheme.rsby set theme cookies and sychronize theme change across mutilple iframe byBroadcastChannel- 2.4 extractor html (--dark/--light) logic from
main.csstodx-component-theme.csslacks:
Big changes with
r#asin others component, sry about that