chore(menu-toggle): updated components to use menu toggle#6566
chore(menu-toggle): updated components to use menu toggle#6566mcoker merged 8 commits intopatternfly:v6from
Conversation
|
Preview: https://patternfly-pr-6566.surge.sh A11y report: https://patternfly-pr-6566-a11y.surge.sh |
There was a problem hiding this comment.
The menu on a menu toggle is showing up to the right
There seems to be a problem if the content is too wide?
Various places I see the toggle scrunched up
| ### New | ||
| ```hbs | ||
| {{> menu-new menu--width="200px" menu--items=( | ||
| entries | ||
| (entry name='super fun' IsDisabled=true) | ||
| (entry name='test 1' thing='try me') | ||
| (entry name='test 2' thing='I\'m here') | ||
| ) | ||
| }} | ||
| ``` |
|
|
||
| // TODO: update alignment for text and action | ||
| // TODO: update to use gap instead of margin | ||
|
|
There was a problem hiding this comment.
Can you make issues for these changes instead of comments here?
There was a problem hiding this comment.
I'm sure some of these are known/purposeful since it's a WIP but since I've reviewed, IMO everything looks good to merge except:
- The HBS helpers have console.logs that need to be removed
- The "New" menu example should be removed
- Overflow menu id's are broken, left a comment below on overflow.hbs
I'd like to merge this if possible so these examples are fixed. Currently they're throwing off the layout of lots of components (this visual regression report has 148 changes as result of this PR) and causing lots of issues with our screenshots. It would be great to fix those. If there is a different way of doing the hbs to make it more streamlined, that could always be done later and the visual regression tool can validate since it should(?) result in no visual changes. wdyt @srambach?
| menu-toggle--IsPlain=true | ||
| menu-toggle--HasKebab=true | ||
| menu-toggle--id=(dasherize data-list--id data-list-item--id 'menu-toggle') | ||
| menu-toggle--aria-label='Dual list item menu toggle'}} |
There was a problem hiding this comment.
| menu-toggle--aria-label='Dual list item menu toggle'}} | |
| menu-toggle--aria-label='Data list item menu toggle'}} |
| ### New | ||
| ```hbs | ||
| {{> menu-new menu--width="200px" menu--items=( | ||
| entries | ||
| (entry name='super fun' IsDisabled=true) | ||
| (entry name='test 1' thing='try me') | ||
| (entry name='test 2' thing='I\'m here') | ||
| ) | ||
| }} | ||
| ``` |
There was a problem hiding this comment.
Did you mean to leave this in?
|
|
||
| {{#>overflow-menu | ||
| overflow-menu-button--aria-label="Expand button group overflow menu" | ||
| overflow-menu--id=(concat toolbar--id '-overflow-menu') menu--IsExpanded=toolbar-overflow-menu-example--IsExpanded |
There was a problem hiding this comment.
| overflow-menu--id=(concat toolbar--id '-overflow-menu') menu--IsExpanded=toolbar-overflow-menu-example--IsExpanded | |
| overflow-menu--id=(concat toolbar--id '-overflow-menu') | |
| menu--IsExpanded=toolbar-overflow-menu-example--IsExpanded |
| {{ternary overflow-menu--id 'id="{{overflow-menu--id}}"' ''}} | ||
| {{ternary overflow-menu--attribute 'id="{{overflow-menu--id}}"' ''}}> |
There was a problem hiding this comment.
ID's are broken
I'm assuming these 2 lines should be something like this instead? Though I tested it and it's not handling the quotes and everything passed to overflow-menu--attribute even though it's wrapped in {{{...}}}.
| {{ternary overflow-menu--id 'id="{{overflow-menu--id}}"' ''}} | |
| {{ternary overflow-menu--attribute 'id="{{overflow-menu--id}}"' ''}}> | |
| {{ternary overflow-menu--id 'id="' overflow-menu--id '"' ''}} | |
| {{{ternary overflow-menu--attribute overflow-menu--attribute ''}}}> |
Just a side note, not a big deal since this isn't shipped code, but IMO if we're gonna change the way these blocks are written, let's do it consistently. TL;DR - we can just write it the way we know works and is in all of the other hbs, and make this kind of update when we can talk about it, do it consistently, and test it.
8ee341d to
cd2e81a
Compare
b6d571b to
bea59c3
Compare
| --#{$menu}--MinWidth: max-content; | ||
| --#{$menu}--MaxWidth: 400ch; |
There was a problem hiding this comment.
Can you explain why these 2 changes are needed and need to be a part of this PR? Also what's the significance of 400ch?
In react, popper currently manages the menu size including min-width, and there are use cases of menus not as a dropdown, so I'm hesitant to make this change as it could have unintended side effects depending on how the menu is being used.
There was a problem hiding this comment.
Menu has no max-width control.
| {{/menu}} | ||
| {{/dropdown}} | ||
| {{> menu-toggle menu-toggle--text='System log' menu-toggle--id=(dasherize log-viewer--id 'menu-toggle')}} | ||
| {{#> menu menu--modifier="pf-m-drilldown pf-m-align-right" menu--attribute='style="--pf-v6-c-menu--Width: 200px;"'}} |
There was a problem hiding this comment.
| {{#> menu menu--modifier="pf-m-drilldown pf-m-align-right" menu--attribute='style="--pf-v6-c-menu--Width: 200px;"'}} | |
| {{#> menu menu--modifier="pf-m-drilldown" menu--attribute='style="--pf-v6-c-menu--Width: 200px;"'}} |
| @@ -0,0 +1,51 @@ | |||
| {{> menu-toggle menu-toggle--text='System log' menu-toggle--id=(dasherize log-viewer--id 'menu-toggle')}} | |||
| {{#> menu menu--modifier="pf-m-drilldown pf-m-align-right" menu--attribute='style="--pf-v6-c-menu--Width: 200px;"'}} | |||
There was a problem hiding this comment.
| {{#> menu menu--modifier="pf-m-drilldown pf-m-align-right" menu--attribute='style="--pf-v6-c-menu--Width: 200px;"'}} | |
| {{#> menu menu--modifier="pf-m-drilldown" menu--attribute='style="--pf-v6-c-menu--Width: 200px;"'}} |
427be15 to
13516ee
Compare
55f35eb to
66dbbd6
Compare
66dbbd6 to
17f5ec3
Compare
17f5ec3 to
8321c1e
Compare
|
OK - here's the report I just ran https://drive.google.com/file/d/1sIz7YyVqInrF6f8M2gjFTjjsjO_1_aR7/view?usp=sharing
|
@srambach Thanks for your help! I'll create an issue and update today. |
|
🎉 This PR is included in version 6.0.0-alpha.133 🎉 The release is available on: Your semantic-release bot 📦🚀 |
closes #6207
BackstopJS Report.pdfLink to the backstop report@mcoker Link to the backstop report