Add sidebar component #177
Conversation
|
Preview available at https://dioxuslabs.github.io/components/pr-preview/pr-177/ |
ealmloff
left a comment
ealmloff
left a comment
There was a problem hiding this comment.
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
| /// - Other attributes are overwritten by the last occurrence. | ||
| /// | ||
| /// TODO: event handler attributes are not merged/combined yet. | ||
| pub fn merge_attributes(lists: Vec<Vec<Attribute>>) -> Vec<Attribute> { |
There was a problem hiding this comment.
Spread attributes are required to be sorted by the attribute name for more efficient diffing (here). We need to maintain that property while merging and can use that property to more efficiently merge attribute lists
| }, | ||
| let open = ctx.open; | ||
| let disabled = ctx.disabled; | ||
| let data_state = use_memo(move || if open() { "open" } else { "closed" }); |
There was a problem hiding this comment.
This is very cheap and nothing expensive depends on it, it doesn't need a memo
| rsx! { | ||
| collapsible::CollapsibleTrigger { attributes: merged, | ||
| {props.children} | ||
| svg { |
There was a problem hiding this comment.
The two CollapsibleTrigger's could be merged with an inline if statement in the block to only render the svg if as is_none()
| 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.
Do we need two open signals/callbacks?
| }; | ||
| } | ||
|
|
||
| if is_mobile() { |
There was a problem hiding this comment.
Rendering a sheet instead of a sidebar on mobile doesn't seem like the right behavior. It looks like shadcn exposes options to adjust the width of the sidebar on mobile or hide the sidebar on mobile instead?
2 participants
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