Skip to content

docs(listbox, listbox-button): adding dom examples with group titles for options#136

Merged
ianmcburnie merged 9 commits intoeBay:gh-pagesfrom
ednihs-yahska:feat/grouped-sequential-lists
Apr 28, 2025
Merged

docs(listbox, listbox-button): adding dom examples with group titles for options#136
ianmcburnie merged 9 commits intoeBay:gh-pagesfrom
ednihs-yahska:feat/grouped-sequential-lists

Conversation

@ednihs-yahska
Copy link
Contributor

@ednihs-yahska ednihs-yahska commented Mar 14, 2025

Description

This PR adds a few examples to the listbox and listbox button mindpatterns.
I created a word doc for listbox and listbox button each to track the accessibility support for these new additions.
Makeup JS PR: makeup/makeup-js#208

Listbox

https://docs.google.com/document/d/1-rxDLUE0qb1qtYv8cjc3WZwi43U5DI0rB7Uy9FIWlaM/edit?tab=t.0

Listbox Buttons

https://docs.google.com/document/d/1sYsj6wDvzeutwMmagy6Gp1C3GN4cDQYAEoG3HKiQK6Y/edit?tab=t.0

I originally started with examples with role=group that nested each role=option. It works well in terms of accessibility in all cases except VoiceOver + Safari. Since it had such a bad support for such a popular combination, I went with current approach without role=group. I have the grouped approach in a different branch, if we want to review it as well.

Please let me know if I can add more information. I will open the PR as a draft at first. Thanks.

I had to modify the gem file to make the project run. I can remove those changes if needed.

Screenshots

Listbox

Screenshot 2025-04-18 at 11 25 07 AM Screenshot 2025-04-18 at 11 24 59 AM

Listbox Buttons

Screenshot 2025-04-18 at 11 25 42 AM Screenshot 2025-04-18 at 11 25 27 AM Screenshot 2025-04-18 at 11 25 20 AM

@ianmcburnie
Copy link
Contributor

ianmcburnie commented Mar 14, 2025

Thank you for this PR. Could you please also add screenshots of the new example to help with context?

It seems that this DOM works out of the box with the makeup listbox and listbox-button modules? Is that right?

@ednihs-yahska
Copy link
Contributor Author

ednihs-yahska commented Mar 14, 2025

Thank you for this PR. Could you please also add screenshots of the new example to help with context?

It seems that this DOM works out of the box with the makeup listbox and listbox-button modules? Is that right?

I found that I did not need to edit the makeup components. I found that the javascript was used from makeup but the html was being used from the _input/listbox/index.html files. I might have misunderstood how the 2 project are setup or used.
I will add the screenshots

@@ -147,6 +149,248 @@ <h3>Selected</h3>
</div>
</div>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have an <hr> here

@@ -186,6 +186,276 @@ <h3>Selected</h3>
</div>
</div>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have an <hr> here for consistency

Copy link
Contributor

@saiponnada saiponnada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM. Few things I would like to suggest

  • Lets add same examples to makeup-js repo(hopefully we wont need this duplicated effort once we consolidate all repos)
  • Lets add playwright UI tests to test these new changes on makeup-js repo.
  • I am having some issues on my end to build this, I couldnt really validate the build.

@ednihs-yahska ednihs-yahska changed the title feat: adding dom for listbox examples with group titles docs(listbox, listbox-button): adding dom examples with group titles for options Apr 2, 2025
@ednihs-yahska ednihs-yahska marked this pull request as ready for review April 2, 2025 23:09
Copy link

@cordeliadillon cordeliadillon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the new styling! One small comment about where to place behavior description but otherwise this looks great to me.

@ianmcburnie
Copy link
Contributor

So sorry for the delay on this on my end. Hoping to get to it this week!

@ianmcburnie
Copy link
Contributor

ianmcburnie commented Apr 14, 2025

I didn't get a chance to review the makeup PR before it was merged: makeup/makeup-js#208.

I see a few things I'd like to change/fix. Mostly minor & non controversial. In interests of speed, I'll just make the changes myself today and push up another PR. Then you can update this PR to incorporate those changes.

@ianmcburnie
Copy link
Contributor

ianmcburnie commented Apr 14, 2025

@ednihs-yahska Did you try CSS columns?

 .column-container [role="listbox"] {
  columns: 2;
}
.header-container {
  break-before: column;
}

It seems to work pretty good for me, and lets us remove the column elements in the DOM and be more responsive. The trick to getting it to work is the break-before: column.

I have it in a PR here if you want to try it out: makeup/makeup-js#212

@ianmcburnie ianmcburnie merged commit 5e8b7d9 into eBay:gh-pages Apr 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants