-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Navigation Component: Style #25258
Comments
@Copons Thanks for following up and summarizing some of these points!
This is an interesting question! For my attempt, I was quite hands-off as far as style goes, but I did give it full height. To be more specific, height (and potentially overflow) styles may be required for it's slide animation transitions to work.
I believe that would be best. I feel like the Navigation component should be primarily responsible for managing and rendering a nested panes/screens, which can be affected either by (internal) "links" or external state. What the Navigation container looks like, what an individual pane/screen looks like, and what a screen may contain should be entirely up to the consumer - which gives us a lot of flexibility and control. |
I'm pretty certain the height doesn't affect the animation, as it would be applied to the main wrapper (which wraps the animation component). Anyway, I guess that full height is kind of implied by a Navigation, and it would be easily overridable by specific cases.
I'm not sure I agree entirely. 🤔 Right now we have two well defined use cases for this component:
We could sort the colors out by providing two very lightweight themes (easily overridable, e.g. by the user's admin theme colors, or by the component consumers own styles), and then we could find a good middle ground between the two use cases. For example, in your "light" example the menu title has a bottom border, that's missing in the "dark" drafts. |
Note in case we forget: currently the text colors are "inverted" compared to the design drafts.
|
Purely in terms of style I think what we have currently is OK. If the technicals are buttoned-up feel free to close this @Copons. |
The
Navigation
component should:Discussion points:
Navigation
beheight: 100%
? Should it be a consumer concern? Should we expose afullHeight
optional prop? cc @psealocktheme
prop? cc @ItsJonQFYI currently the navigation looks like this:
And it can be tested in
npm run storybook:dev
.The text was updated successfully, but these errors were encountered: