-
Notifications
You must be signed in to change notification settings - Fork 170
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
Add config for aria labels and text labels, and fix test #303
Conversation
@@ -25,6 +25,6 @@ describe('getCurrentSlideIndex', () => { | |||
const max = 10 | |||
const results = mapNumberToRange({ val, min, max }) | |||
|
|||
expect(results).toBe(5) | |||
expect(results).toBe(7) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes #302.
src/components/Pagination.ts
Outdated
@@ -25,7 +27,7 @@ const Pagination = () => { | |||
'carousel__pagination-button': true, | |||
'carousel__pagination-button--active': isActive(slide), | |||
}, | |||
'aria-label': `Navigate to slide ${slide + 1}`, | |||
'aria-label': config.labels?.ariaNavigateToSlide + ' ' + (slide + 1).toString(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used some kind of template to allow for simpler translations of Item X of Y
, we perhaps want something similar here, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, It's better to have a unified API
src/components/Icon.ts
Outdated
const iconTitle = (props.title | ||
|| (config.labels?.iconAriaLabels ? config!.labels!.iconAriaLabels![iconName]! as string : undefined) | ||
|| iconName | ||
) as string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still uses the icon name as a fallback but prefers the configured title. Not sure if that's alright the way it is or if we should inject the translated aria label via props instead.
@thormeier Thank you for your awesome work. :) |
Fixes #302 and implements #278.
I tried to stick to the code style as much as possible. I had to type the icons more explicitly for them to be translateable, but in turn, I also added translations for the icons. I added an example to the docs and added the new object to the config table. We might want to think of a better way to show the new config object since I couldn't indent it in the markdown table nicely.
Thank you very much for this library!