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

fix(pagination): correct labelling discrepancies between versions #1417

Merged
merged 4 commits into from Nov 15, 2018
Merged

fix(pagination): correct labelling discrepancies between versions #1417

merged 4 commits into from Nov 15, 2018

Conversation

dakahn
Copy link
Contributor

@dakahn dakahn commented Nov 9, 2018

Closes #1362

Fixes up some discrepancies between the two different versions of pagination causing labels to be applied incorrectly.

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

Wow you are up late @dakahn! LGTM 👍 - Thanks!

@@ -2,9 +2,9 @@
<div class="bx--pagination__left">
<div class="bx--select{{#is version "v2"}} bx--select--inline{{/is}}">
{{#is version "v2"}}
<label for="select-id-pagination" class="bx--pagination__text">Items per page: </label>
<label for="items-per-page" class="bx--pagination__text">Items per page: </label>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the correct label? <label> content should be the same as aria-label for select if we are using the for attribute, I thought? Let me know if I'm off here 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally adding redundancy here, you're right. There's only an implicit label in version 2, so the added aria-label is DAP errors in other versions.

@dakahn dakahn merged commit 49b2a1f into carbon-design-system:master Nov 15, 2018
@carbon-bot
Copy link
Contributor

🎉 This PR is included in version 9.53.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

joshblack pushed a commit to joshblack/carbon that referenced this pull request May 2, 2019
…system#1417)

* fix: Add listener to outside of modal.

* fix: Update focus ref.

* fix: Change ref naming to more closely mimic the Modal component.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants