fix(Select): grouped Select generates invalid HTML #4919
Conversation
|
PF4 preview: https://patternfly-react-pr-4919.surge.sh |
Codecov Report
@@ Coverage Diff @@
## master #4919 +/- ##
=======================================
Coverage 52.54% 52.55%
=======================================
Files 526 526
Lines 9575 9579 +4
Branches 3533 3536 +3
=======================================
+ Hits 5031 5034 +3
Misses 3900 3900
- Partials 644 645 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
The dividers are not wrapped in an li tag.
I'm a little afraid to comment that, and i don't know if it's invalid HTML or not without looking it up.
If we need to wrap the dividers, we need to make sure that the keyboard interactions still work in the typeahead variants.
| </div> | ||
| <li {...props} className={css(styles.selectMenuGroupTitle, className)} id={titleId}> | ||
| {label} | ||
| <ul className={css(styles.selectMenuGroup)} role="listbox"> |
There was a problem hiding this comment.
I think you should only change that children is wrapped by <ul className={css(styles.selectMenuGroup)} role="listbox">, and leave the outer elements the way they were
There was a problem hiding this comment.
That's what the issue description suggests to do, but if you read the specification of what can be child of UL is not contain divs.
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/ul
SelectGroups is direct child of UI, and should not contain any divs in other words.
There was a problem hiding this comment.
The DOM should look similar to this (copied from Core example):
There was a problem hiding this comment.
Also the changes break the styling a bit, additional indentation etc
https://patternfly-react-pr-4919.surge.sh/components/select#grouped-checkbox-input
There was a problem hiding this comment.
Ack I see it now in the HTML docs. I will adjust it tomorrow.
Thanks for the review.
There was a problem hiding this comment.
I wonder if it's a concious decision though that nested ul li were not used in the HTML docs.
For me it makes sense that the outer items would be also a list (ul) since there indeed part of a list.
@mcarrano I guess you can answer this one :) For example this https://patternfly-react-pr-4919.surge.sh/components/select/#grouped-single
There was a problem hiding this comment.
@mcoker can you answer the question above?
There was a problem hiding this comment.
@jschuler is right with the screenshot posted above. I left the same basic comment here - #4919 (comment)
FWIW you can find the same basic structure in the dropdown, or any menu with groups/titles https://www.patternfly.org/v4/components/dropdown/react/with-groups/
div/section.section
div/h1-h6.section-title
ul
li
li
....
Good catch, it's essentially the same problem, do you want me to fix it within this PR, on send another one? |
ul element can only have li's as children. Fixes patternfly#4917
I needed to rebase - had merge conflicts. Sorry :( |
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
Fixes #4917