Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#137] Make ui_tabs accessible #139

Merged
merged 4 commits into from
Oct 23, 2024
Merged

Conversation

hannahbit
Copy link
Contributor

Fixes the ui_tabs not following accessibility guidelines.

See issue: #137

@hannahbit
Copy link
Contributor Author

The gap is now slightly smaller. This is because the default bitstyles does not have a u-gap-s2. The closest available options are u-gap-s3 (which I chose) and u-gap-m. Which would be bigger.
I thought having a little smaller gap might be fine? WDYT?

before now
Screenshot 2024-10-16 at 12 33 42 Screenshot 2024-10-16 at 12 35 49

@andreasknoepfle
Copy link
Member

andreasknoepfle commented Oct 17, 2024

I think this would be just needing a CHANGELOG entry and then be good to go.

I thought having a little smaller gap might be fine? WDYT?

Since one can always set another gap class and overwrite the default one with it it should be fine imho.

@klappradla
Copy link
Member

The code and the generated docs look fine 👍

What does not work for me is the demo application though. Is it known to be broken or is this a new thing?

@hannahbit
Copy link
Contributor Author

What does not work for me is the demo application though. Is it known to be broken or is this a new thing?

It's the same on main for me and unrelated to the changes made in here. The demo needing a rework is already a known issue.

@hannahbit
Copy link
Contributor Author

@andreasknoepfle @klappradla Thanks for the reviews 🧡

I updated the changelog. Can you take another look?

CHANGELOG.md Outdated
@@ -2,6 +2,8 @@

## Unreleased

- Changed `ui_tabs` and `ui_tab_button` to follow accessibility guidelines. As a side effect the gap between the tabs is now a little smaller.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe worth mentioning what you wrote in the issue

replace the <ul> with a <div role="tablist">

Copy link
Member

@klappradla klappradla left a comment

Choose a reason for hiding this comment

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

Ready for 🌊 I'd say.

CHANGELOG.md Outdated Show resolved Hide resolved
@hannahbit hannahbit merged commit eb93701 into main Oct 23, 2024
3 checks passed
@hannahbit hannahbit deleted the feature/137-make-ui_tabs-accessible branch October 23, 2024 11:25
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.

3 participants