geido commented on code in PR #30818:
URL: https://github.com/apache/superset/pull/30818#discussion_r1840150055
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -197,15 +197,20 @@ function SelectPageSize({
onChange,
}: SelectPageSizeRendererProps) {
return (
- <span className="dt-select-page-size form-inline">
- {t('page_size.show')}{' '}
+ <span
+ className="dt-select-page-size form-inline"
+ role="group"
+ aria-label={t('Select page size')}
+ >
+ {t('Show')}{' '}
<select
className="form-control input-sm"
value={current}
onBlur={() => {}}
onChange={e => {
onChange(Number((e.target as HTMLSelectElement).value));
}}
+ aria-label={t('Show entries per page')}
Review Comment:
I think it should be enough to have just this aria-label here and set the
number of entries here (while removing the aria-label from the individual
options). Otherwise, the screen reader is reading "Show 10 entries per page,
Show entries per page" which I think it is unnecessary. Also, on Firefox, when
inspecting for accessibility I am getting this:
<img width="754" alt="Screenshot 2024-11-13 at 16 09 29"
src="https://github.com/user-attachments/assets/0c44ab6d-e4ce-4e32-9c7c-f4e755277976";>
I believe we need to have a label that labels the Select input. We can
probably change the structure a bit so to keep the same layout but have the
proper Label element and accessibility properties. What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
