fix(Menu): Menu width and Submenu indicator fix #1831
Conversation
adding submenu indicator icon
…x/menu-submenu-issues
bcalvery
requested review from
dvdzkwsk,
kuzhelov,
layershifter,
levithomason,
miroslavstastny and
mnajdova
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1831 +/- ##
==========================================
- Coverage 69.9% 69.87% -0.04%
==========================================
Files 888 890 +2
Lines 7775 7783 +8
Branches 2244 2275 +31
==========================================
+ Hits 5435 5438 +3
- Misses 2330 2335 +5
Partials 10 10
Continue to review full report at Codecov.
|
add examples to test width
|
I think that when using the RTL mode, the submenu arrow should point to the opposite direction. Also, the expanded submenu should go to the opposite side in RTL mode. |
…x/menu-submenu-issues
In teams theme, I don't think it should shift menu items without icons. I don't think it is a common case but the few examples I found do not shift.
|
| {menu && | ||
| Icon.create(indicatorWithDefaults, { | ||
| defaultProps: { | ||
| name: vertical ? 'stardust-arrow-end' : 'stardust-arrow-down', | ||
| name: vertical ? 'chevron-right' : 'chevron-down', |
There was a problem hiding this comment.
I think the stardust prefix for icons is used to indicate when a different icons is used per theme. So I think you might want to update or add a reference.
https://github.com/stardust-ui/react/blob/master/packages/react/src/themes/teams/index.tsx
There was a problem hiding this comment.
I'm not sure how I would I go about changing modifying 'stardust-arrow-down' and 'stardust-arrow-end' just in the context of Menu.
Would change it for every case of 'stardust-arrow-down and -right' which isn't what I want - I don't think it is at least.
Doing this would mean new assets are needed for the default Stardust theme right?
| @@ -255,8 +256,8 @@ class MenuItem extends AutoControlledComponent<WithAsProp<MenuItemProps>, MenuIt | |||
| <> | |||
| <Ref innerRef={this.menuRef}> | |||
| <Popper | |||
| align={vertical ? 'top' : 'start'} | |||
| position={vertical ? 'after' : 'below'} | |||
| align={vertical ? 'top' : rtl ? 'end' : 'start'} | |||
There was a problem hiding this comment.
Stardust noobie question here for @codepretty, @levithomason , etc.: would this be better done within Popper? Making Popper itself respect RTL settings would eliminate this work on the consumer's end, which may otherwise be forgotten.
There was a problem hiding this comment.
I'm not sure. Popper seems to be a bit more generic - it depends on the element using it to feed it position information, and the positioning of Menu is affected by RTL possibly differently than other items using Popper. @levithomason ?
https://github.com/stardust-ui/react/blob/master/packages/react/src/lib/positioner/Popper.tsx
There was a problem hiding this comment.
But Popper should respect rtl, this works perfectly if you use the Popup component for example:
Let me dive into this tomorrow and see what's happening
There was a problem hiding this comment.
@mnajdova Menu is using "Popper" not "Popup" - maybe that is part of the difference?
There was a problem hiding this comment.
I am trying to swap in Popup for Popper in MenuItem, and it just is not working:
Popper has a targetRef property, while Popup seems to have a target property which is passed to Popper in Popup), but target of Popup doesn't want to accept {this.itemRef}.
There was a problem hiding this comment.
@bcalvery, i think you have a good solution. i think it probably makes sense to keep this rtl logic where you have it unless we know how popper will be used in all cases, it might not make sense to prematurely move this logic to popper.
…x/menu-submenu-issues
…x/menu-submenu-issues
…x/menu-submenu-issues
| @@ -11,6 +11,8 @@ export const icons: ThemeIcons = { | |||
| 'stardust-arrow-down': fontIcon('25BE'), | |||
| 'stardust-arrow-end': fontIcon('25B8'), | |||
| 'stardust-arrow-up': fontIcon('25B4'), | |||
| 'stardust-menu-arrow-down': fontIcon('25BE'), | |||
| 'stardust-menu-arrow-end': fontIcon('25B8'), | |||
There was a problem hiding this comment.
I think it's nice to scope these icons per component, so people would ideally configure via the theme which icons should appear on which components. 👍
…x/menu-submenu-issues
| content: ({ props: p }): ICSSInJSStyle => { | ||
| const widthAdjust = (p.icon ? 26 : 0) + (p.menu ? 16 : 0) | ||
| return { | ||
| whiteSpace: 'normal', |
There was a problem hiding this comment.
I think the default value is already normal.. is this white-space property overriding something?
There was a problem hiding this comment.
The root element has whiteSpace: 'nowrap' and that seems to affect all children, but the text within the "content" should wrap if necessary, so this returns it to 'normal'.
…x/menu-submenu-issues
…-ui/react into fix/menu-submenu-issues
Objective: Improve width behavior and update indicator icon to chevron type.
Before:
After:
