Update nav styles to match core designs #24987
Conversation
joshuatf
requested review from
ajitbohra,
chrisvanpatten and
youknowriad
as code owners
ZebulanStanphill
added
Needs Accessibility Feedback
|
I think it makes sense to go with the dark theme and write CSS to the spec pasted above. The reason is because both the FSE and WooCommerce usage won't be in the context of the wp-admin sidebar. Should a time come to examine that change, it will likely be in conjunction with a change to the top nav bar. FSE and WooCommerce will occupy full screens. This means that this Navigation component won't need to match a background theme color found in wp-admin sidebar or top nav bar.
I do think that focus style colors should respect the theme. But ping @jameskoster to confirm.
|
Agreed! |
joshuatf
force-pushed
the
update/nav-styles
branch
from
2cb2ee9 to
760b0da
Compare
|
Thanks for the input @jameskoster and @psealock! This has been rebased and ready for another review. Dark styles are added and currently this just respects the built-in theme colors of the GB buttons. If there are more theming issues, I suggest we handle in a follow-up. This PR also adds menu titles (section titles) and a secondary menu to the nav story to illustrate potential use cases. |
joshuatf
changed the title
There was a problem hiding this comment.
This is looking really nice @joshuatf. I just left a minor comment.
This PR also adds menu titles (section titles) and a secondary menu to the nav story to illustrate potential use cases.
This is the pattern we set for WooCommerce, but I think its not one we should settle on because it falls short in several ways:
- The consuming app needs to have knowledge of any secondary menus in its render function as well as at the point of constructing data.
- This works well for a single secondary menu on the navigation root. What if there is more than one? What if those secondary menu titles need to be dynamic?
I don't think rendering these secondary menus or groupings should be completely the responsibility of the consumer. I created an issue to track that feature in #24992 and I think it'd be best to explore that outside of this PR (even if we do go this route in the end).
| @@ -73,13 +73,14 @@ const Navigation = ( { activeItemId, children, data, rootTitle } ) => { | |||
| } | |||
|
|
|||
| return ( | |||
| <Button | |||
| <BackButtonUi | |||
There was a problem hiding this comment.
Other names have a capitalized I as in BackButtonUI.
joshuatf
force-pushed
the
update/nav-styles
branch
from
760b0da to
d8afbb9
Compare
|
Thanks for the review @psealock. Updated the ui component name based on your feedback.
The consumer can decide if there is more than one. In the example, I just added I do think this should be more up to the consumer and prefer a slightly more compositional approach, though that does come with some drawbacks. We can discuss in #25035 |
| .map( ( item ) => | ||
| renderMenuItem( item ) | ||
| ) } | ||
| </NavigationMenu> |
There was a problem hiding this comment.
The consumer can decide if there is more than one. In the example, I just added isSecondary but this could be a slug or any other property to discern various menus. The title can also be dynamic here- also up to the consumer.
This is true, and this block is a nice example of the consumer having control. What if the entire navigation has 10 of these submenus throughout the hierarchy? Looking at the designs for FSE, its possible that may be the case. You'd need a block like this for every single submenu. level.children is getting filtered with each block. I think a better way exists and worth exploring.
Description
Related to https://github.com/woocommerce/navigation/pull/68 - this PR updates to the latest design exploration for the nav component ( https://www.figma.com/file/e4tLacmlPuZV47l7901FEs/WordPress-Design-Library?node-id=1456%3A72 ).
Screenshots