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

LPD-17368 Accessibility issue in Pagination Picker #5764

Merged
merged 2 commits into from
Feb 19, 2024

Conversation

ilzamcmed
Copy link
Member

@ilzamcmed ilzamcmed force-pushed the LPD-17368 branch 2 times, most recently from dfd3e42 to 61db8c3 Compare February 8, 2024 20:13
@@ -230,6 +233,7 @@ export const ClayPaginationBarWithBasicItems = ({
<Picker
activeDelta={activeDelta}
aria-describedby={ariaDescribedby}
aria-label={labels.pagination}
Copy link
Member

Choose a reason for hiding this comment

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

We need a better name for this variable. This is the aria label for the pagination delta picker. In general our props that are used only as an aria-label, include aria label in the name. If we use this as the title (tooltip) then we could not include ariaLabel in the name. The date picker and color picker include an aria-labels prop that adds all aria labels to that object. So that's another option, but since we only have one aria-label currently that might be too much. So maybe pageDeltaPickerAriaLabel or itemsPerPagePickerTitle of it makes sense to add this to a title/tooltip then pageDeltaPickerTitle or itemsPerPagePickerTitle.

Additionally, we need to think about the default label. Maybe Items Per Page. This would result in the screen reader reading: "Items Per Page Combo box, 20 items." @marcoscv-work What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

We used to use ariaLabels to group all of this due to internationalization but last year we changed to using a name that could group ariaLabels and other texts, so we are using messages now.

I wouldn't add it now because it would introduce a new separate API, but I would keep it as is using labels and move it to messages in the main version.

Copy link
Member

Choose a reason for hiding this comment

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

Additionally, we need to think about the default label. Maybe Items Per Page. This would result in the screen reader reading: "Items Per Page Combo box, 20 items." @marcoscv-work What do you think?

Sounds much better for me "Items Per Page" for the picker, label much more help

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @marcoscv-work.

So the variable should probably be called itemsPerPagePickerAriaLabel and the default value should be "Items Per Page". Does that sound okay?

@ethib137
Copy link
Member

ethib137 commented Feb 9, 2024

Since we are adding a new language key in this PR, when we release this in Portal we will need to simultaneously add a new language key to language.properties and add the additional key to all usages of this component. @ilzamcmed can you create a ticket for this? @matuzalemsteles do we already have a process for making sure we apply these additional changes to portal as part of the Clay release?

@matuzalemsteles
Copy link
Member

do we already have a process for making sure we apply these additional changes to portal as part of the Clay release?

In fact we have no process for this, when we add a new key, we make this key optional when it is inside the object to avoid a breaking change, for example when the component has already declared labels but a new key was added this would cause a build error. So this is treated as breaking change.

We normally do this but we don't update all DXP use cases due to workforce so as not to delay the release. But we can include this or we can start prioritizing the work on integrating Liferay.Language with the portal so that we only need to change in one place.

@ethib137
Copy link
Member

Thanks @matuzalemsteles , that makes sense.

But we can include this or we can start prioritizing the work on integrating Liferay.Language with the portal so that we only need to change in one place.

I think it makes more sense to prioritize integrating Liferay.Language, like we've discussed before. Something like a ClayMessageProvider. Do you remember where we discussed this previously. I think it was on a ticket or something. I'll create a new story for this and start working on the requirements.

@matuzalemsteles
Copy link
Member

I think it makes more sense to prioritize integrating Liferay.Language, like we've discussed before. Something like a ClayMessageProvider. Do you remember where we discussed this previously. I think it was on a ticket or something. I'll create a new story for this and start working on the requirements.

Yes, something like that. We have a ticket that discussed this but I think we need to create another one with this type of story. https://liferay.atlassian.net/browse/LPS-190534

@ilzamcmed ilzamcmed force-pushed the LPD-17368 branch 2 times, most recently from 5a50bf4 to 576591b Compare February 15, 2024 20:44
Copy link
Member

@matuzalemsteles matuzalemsteles left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@ethib137 ethib137 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@matuzalemsteles matuzalemsteles merged commit 09ecafc into liferay:master Feb 19, 2024
4 checks passed
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