Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
github-actions[bot] commented on PR #26602: URL: https://github.com/apache/superset/pull/26602#issuecomment-2064455994 Ephemeral environment shutdown and build artifacts deleted. -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
geido merged PR #26602: URL: https://github.com/apache/superset/pull/26602 -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
eulloa10 commented on code in PR #26602:
URL: https://github.com/apache/superset/pull/26602#discussion_r1567863654
##
superset-frontend/src/components/MetadataBar/MetadataBar.test.tsx:
##
@@ -265,3 +265,27 @@ test('correctly renders the tags tooltip', async () => {
);
});
});
+
+test('renders StyledItem with role="button" when onClick is defined', () => {
+ const onClick = jest.fn();
+ const items = [
+{ ...ITEMS[0], onClick },
+{ ...ITEMS[1], onClick },
+ ];
+ render();
+
+ const styledItems = screen.getAllByRole('button');
+
+ styledItems.forEach(styledItem => {
+expect(styledItem).toHaveAttribute('role', 'button');
+ });
+});
+
+test('renders StyledItem with role=undefined when onClick is not defined', ()
=> {
+ const items = [ITEMS[0], ITEMS[1]];
+ render();
+ const styledItem = screen
+.getByText(DASHBOARD_TITLE)
+.closest('.metadata-text');
+ expect(styledItem).not.toHaveAttribute('role');
Review Comment:
This will be changed to:
```
const styledItems = screen.queryAllByRole('button');
expect(styledItems.length).toBe(0);
```
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
eulloa10 commented on code in PR #26602:
URL: https://github.com/apache/superset/pull/26602#discussion_r1567863322
##
superset-frontend/src/components/MetadataBar/MetadataBar.test.tsx:
##
@@ -265,3 +265,27 @@ test('correctly renders the tags tooltip', async () => {
);
});
});
+
+test('renders StyledItem with role="button" when onClick is defined', () => {
+ const onClick = jest.fn();
+ const items = [
+{ ...ITEMS[0], onClick },
+{ ...ITEMS[1], onClick },
+ ];
+ render();
+
+ const styledItems = screen.getAllByRole('button');
+
+ styledItems.forEach(styledItem => {
+expect(styledItem).toHaveAttribute('role', 'button');
+ });
Review Comment:
Yes, this will be changed to:
```
const styledItems = screen.getAllByRole('button');
expect(styledItems.length).toBe(2);
```
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
geido commented on code in PR #26602:
URL: https://github.com/apache/superset/pull/26602#discussion_r1567639042
##
superset-frontend/src/components/MetadataBar/MetadataBar.test.tsx:
##
@@ -265,3 +265,27 @@ test('correctly renders the tags tooltip', async () => {
);
});
});
+
+test('renders StyledItem with role="button" when onClick is defined', () => {
+ const onClick = jest.fn();
+ const items = [
+{ ...ITEMS[0], onClick },
+{ ...ITEMS[1], onClick },
+ ];
+ render();
+
+ const styledItems = screen.getAllByRole('button');
+
+ styledItems.forEach(styledItem => {
+expect(styledItem).toHaveAttribute('role', 'button');
+ });
+});
+
+test('renders StyledItem with role=undefined when onClick is not defined', ()
=> {
+ const items = [ITEMS[0], ITEMS[1]];
+ render();
+ const styledItem = screen
+.getByText(DASHBOARD_TITLE)
+.closest('.metadata-text');
+ expect(styledItem).not.toHaveAttribute('role');
Review Comment:
Maybe we can do the opposite here and verify that the length of elements
with `role=button` is zero?
##
superset-frontend/src/components/MetadataBar/MetadataBar.test.tsx:
##
@@ -265,3 +265,27 @@ test('correctly renders the tags tooltip', async () => {
);
});
});
+
+test('renders StyledItem with role="button" when onClick is defined', () => {
+ const onClick = jest.fn();
+ const items = [
+{ ...ITEMS[0], onClick },
+{ ...ITEMS[1], onClick },
+ ];
+ render();
+
+ const styledItems = screen.getAllByRole('button');
+
+ styledItems.forEach(styledItem => {
+expect(styledItem).toHaveAttribute('role', 'button');
+ });
Review Comment:
This seems redundant. Maybe we can check whether these elements exist by
checking the length?
##
superset-frontend/src/components/Tags/Tag.tsx:
##
@@ -47,6 +50,15 @@ const Tag = ({
const handleClose = () => (index ? onDelete?.(index) : null);
+ let whatRole;
+ if (onClick) {
+if (!id) {
+ whatRole = 'button';
+} else {
+ whatRole = 'link';
+}
+ }
Review Comment:
```suggestion
const whatRole = onClick ? (!id ? 'button' : 'link') : undefined;
```
##
superset-frontend/src/components/Label/index.tsx:
##
@@ -93,6 +93,7 @@ export default function Label(props: LabelProps) {
return (
undefined}>
Review Comment:
@michael-s-molina we are keeping this here unless you give us the green
light to remove it
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
eulloa10 commented on code in PR #26602:
URL: https://github.com/apache/superset/pull/26602#discussion_r1558772499
##
superset-frontend/src/explore/components/controls/FixedOrMetricControl/index.jsx:
##
@@ -127,7 +127,7 @@ export default class FixedOrMetricControl extends
React.Component {
undefined}>
Review Comment:
This was introduced as part of a migration from Bootstrap to AntD components
in this
[PR](https://github.com/apache/superset/pull/12920/files#diff-a7be32a17ea0cae5c0409a1c8ef7abd669132a56052f97a24aac78570f074cef).
However, there doesn't seem to be additional context as to why this `onClick`
is returning `undefined`. Should we leave it as is and not remove the `onClick`?
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
eulloa10 commented on code in PR #26602:
URL: https://github.com/apache/superset/pull/26602#discussion_r1558772499
##
superset-frontend/src/explore/components/controls/FixedOrMetricControl/index.jsx:
##
@@ -127,7 +127,7 @@ export default class FixedOrMetricControl extends
React.Component {
undefined}>
Review Comment:
This was introduced as part of a migration from Bootstrap to AntD components
in this
[PR](https://github.com/apache/superset/pull/12920/files#diff-a7be32a17ea0cae5c0409a1c8ef7abd669132a56052f97a24aac78570f074cef).
However, there doesn't seem to be additional context as to why this `onClick`
is returning `undefined`.
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
geido commented on code in PR #26602:
URL: https://github.com/apache/superset/pull/26602#discussion_r1557961896
##
superset-frontend/src/components/Icons/AntdEnhanced.tsx:
##
@@ -25,9 +25,10 @@ import IconType from './IconType';
const AntdEnhancedIcons = Object.keys(AntdIcons)
.filter(k => !k.includes('TwoTone'))
.map(k => ({
-[k]: (props: IconType) => (
-
-),
+[k]: (props: IconType) => {
+ const whatRole = props.onClick ? 'button' : props?.role || 'img';
Review Comment:
```suggestion
const whatRole = props?.onClick ? 'button' : 'img';
```
##
superset-frontend/src/components/Label/index.tsx:
##
@@ -93,6 +93,7 @@ export default function Label(props: LabelProps) {
return (
undefined}>
Review Comment:
Can we git blame this and see why it was done in the first place?
##
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/CrossFilters/CrossFilterTag.tsx:
##
@@ -81,6 +84,7 @@ const CrossFilterTag = (props: {
`}
closable
onClose={() => removeCrossFilter(filter.emitterId)}
+ closeIcon={customCloseIcon}
Review Comment:
All the time you have a closable Tag, the x should get a role button, so I
believe it makes sense to move this within the `Tag` component
##
superset-frontend/src/components/Icons/Icon.tsx:
##
@@ -68,10 +68,13 @@ export const Icon = (props: IconProps) => {
};
}, [fileName, ImportedSVG]);
+ const whatRole = props?.onClick ? 'button' : iconProps?.role || 'img';
Review Comment:
```suggestion
const whatRole = props?.onClick ? 'button' : 'img';
```
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
geido commented on code in PR #26602:
URL: https://github.com/apache/superset/pull/26602#discussion_r1557956067
##
superset-frontend/plugins/legacy-preset-chart-deckgl/src/components/Legend.tsx:
##
@@ -106,6 +106,7 @@ const Legend = ({
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
eulloa10 commented on code in PR #26602:
URL: https://github.com/apache/superset/pull/26602#discussion_r1551863711
##
superset-frontend/plugins/legacy-preset-chart-deckgl/src/components/Legend.tsx:
##
@@ -106,6 +106,7 @@ const Legend = ({
https://github.com/apache/superset/pull/26602/files#r1504672422
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
github-actions[bot] commented on PR #26602: URL: https://github.com/apache/superset/pull/26602#issuecomment-2031185277 @geido Ephemeral environment spinning up at http://34.218.74.195:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup. -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
geido commented on PR #26602: URL: https://github.com/apache/superset/pull/26602#issuecomment-2031171927 /testenv up -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
geido commented on code in PR #26602:
URL: https://github.com/apache/superset/pull/26602#discussion_r1504696661
##
superset-frontend/src/SqlLab/components/TableElement/index.tsx:
##
@@ -345,6 +345,7 @@ const TableElement = ({ table, ...props }:
TableElementProps) => {
+
Review Comment:
Same here. I think each item should have a button role instead of the Menu
itself?
##
superset-frontend/src/SqlLab/components/QueryLimitSelect/index.tsx:
##
@@ -72,7 +72,11 @@ function renderQueryLimit(
return (
{[...new Set(limitDropdown)].map(limit => (
- setQueryLimit(limit)}>
+ {
trigger={['click']}
overlayStyle={{ zIndex: theme.zIndex.max }}
overlay={
- onChange(key)}>
+ = ({
+
Review Comment:
Same as other comments about the Menu getting the button role instead of its
items
##
superset-frontend/src/components/Datasource/CollectionTable.tsx:
##
@@ -312,12 +312,12 @@ export default class CRUDCollection extends
React.PureComponent<
renderSortIcon(col: string) {
if (this.state.sortColumn === col && this.state.sort === SortOrder.asc) {
- return ;
+ return ;
}
if (this.state.sortColumn === col && this.state.sort === SortOrder.desc) {
- return ;
+ return ;
}
-return ;
+return ;
Review Comment:
Wondering if we can modify the base `Icons` component to support the
conditional button role when `onClick` is used
##
superset-frontend/src/dashboard/components/FiltersBadge/FilterIndicator/index.tsx:
##
@@ -40,6 +40,7 @@ const FilterIndicator: FC = ({
const resultValue = getFilterValueForDisplay(value);
return (
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
geido commented on code in PR #26602:
URL: https://github.com/apache/superset/pull/26602#discussion_r1504671762
##
superset-frontend/plugins/legacy-preset-chart-deckgl/src/components/Legend.tsx:
##
@@ -106,6 +106,7 @@ const Legend = ({
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
geido commented on code in PR #26602:
URL: https://github.com/apache/superset/pull/26602#discussion_r1504671762
##
superset-frontend/plugins/legacy-preset-chart-deckgl/src/components/Legend.tsx:
##
@@ -106,6 +106,7 @@ const Legend = ({
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
mistercrunch commented on code in PR #26602:
URL: https://github.com/apache/superset/pull/26602#discussion_r1482171641
##
superset-frontend/src/dashboard/components/menu/DownloadMenuItems/DownloadAsPdf.tsx:
##
@@ -29,7 +29,7 @@ export default function DownloadAsPdf({
return (
-
+
Review Comment:
Wondering if we can just bring the onClick and tabIndex to the parent
(Menu.Item) and get rid of that child `` altogether here
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
mistercrunch commented on PR #26602: URL: https://github.com/apache/superset/pull/26602#issuecomment-1933034872 The beauty is that a single PR handling both `Menu` and `MenuItem` might be fairly simple and tackle a lot of the changes here in a very simple way. -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
eulloa10 commented on PR #26602: URL: https://github.com/apache/superset/pull/26602#issuecomment-1931255871 @mistercrunch thank you, appreciate the additional context here and feedback. I am going to use your approach and also split the changes into multiple PRs. -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
mistercrunch commented on PR #26602: URL: https://github.com/apache/superset/pull/26602#issuecomment-1928561690 Similarly, looking at all the many `` and `` entries here, you could really just force role="button" if/when `onClick` is defined, and not have to sprinkle `role="button"` everywhere. We should have a hard rule to ALWAYS use antd component through the wrappers in `src/components/` meaning we always do this lower level stuff from that central point. -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
mistercrunch commented on code in PR #26602:
URL: https://github.com/apache/superset/pull/26602#discussion_r1479083009
##
superset-frontend/src/dashboard/components/PublishedStatus/index.jsx:
##
@@ -65,6 +65,7 @@ export default class PublishedStatus extends React.Component {
title={draftButtonTooltip}
>
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
mistercrunch commented on code in PR #26602:
URL: https://github.com/apache/superset/pull/26602#discussion_r1479082544
##
superset-frontend/src/components/CachedLabel/index.tsx:
##
@@ -44,6 +44,7 @@ const CacheLabel: React.FC = ({
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
mistercrunch commented on PR #26602: URL: https://github.com/apache/superset/pull/26602#issuecomment-1928545991 Is there an opportunity as part of this PR to improve accessibility by reusing (or introducing if they don't exist already) low-level reusable components in `src/components` and make those accessible? The original idea with `src/components` was [amongst other things] to offer a subset of `antd` (and native React like `` or `button`) components that are more opinionated on properties (offer only a subset of properties) and used/reused consistently across Superset. Say if we always use `src.components.Button` and that one always has a `role`, then no need to sprinkle `role` in a hundred places (?) Similar with `antd.MenuItem`, if we have a nice little abstraction where you can't get it wrong, then no need to go low level across the codebase, simply in `src/components/` -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
github-actions[bot] commented on PR #26602: URL: https://github.com/apache/superset/pull/26602#issuecomment-1928434782 Ephemeral environment shutdown and build artifacts deleted. -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
eulloa10 closed pull request #26602: refactor: add "button" role to clickable UI elements for improved accessibility URL: https://github.com/apache/superset/pull/26602 -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
geido commented on PR #26602: URL: https://github.com/apache/superset/pull/26602#issuecomment-1910771933 @eulloa10 both Cypress tests and unit tests are failing as you changed some of the roles associated to elements. Please check the errors individually and update the tests according to the new roles. If you need more help you can find me in [Apache Superset Slack](https://join.slack.com/t/apache-superset/shared_invite/zt-2be0drwz8-bxPfkdz28ozzk1Iox29ufg) -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
eulloa10 commented on PR #26602: URL: https://github.com/apache/superset/pull/26602#issuecomment-1909255071 Thanks @geido. I looked through the failing unit tests (17 tests) to start, but I am confused on how the changes I made are contributing to the failing tests. What is the best way to approach debugging these? -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
geido commented on PR #26602: URL: https://github.com/apache/superset/pull/26602#issuecomment-1906714730 Thanks for this @eulloa10. Let me know if I can help with the failing test cases -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
eulloa10 commented on PR #26602: URL: https://github.com/apache/superset/pull/26602#issuecomment-1900806856 I am looking into the failing test cases. -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
github-actions[bot] commented on PR #26602: URL: https://github.com/apache/superset/pull/26602#issuecomment-1899502697 @eschutho Ephemeral environment spinning up at http://34.222.59.163:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup. -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
eschutho commented on PR #26602: URL: https://github.com/apache/superset/pull/26602#issuecomment-1899499521 /testenv up -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
eulloa10 commented on PR #26602: URL: https://github.com/apache/superset/pull/26602#issuecomment-1897716765 Thank you for reviewing @justinpark. I made the requested changes and they are ready to be reviewed. -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
github-actions[bot] commented on PR #26602: URL: https://github.com/apache/superset/pull/26602#issuecomment-1897670782 @eschutho Ephemeral environment spinning up at http://35.162.104.150:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup. -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
eschutho commented on PR #26602: URL: https://github.com/apache/superset/pull/26602#issuecomment-1897668843 /testenv up -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
justinpark commented on code in PR #26602:
URL: https://github.com/apache/superset/pull/26602#discussion_r1456523063
##
superset-frontend/src/explore/components/controls/VizTypeControl/VizTypeGallery.tsx:
##
@@ -478,6 +478,7 @@ const Selector: React.FC<{
key={selector}
name={selector}
className={cx(className, isSelected && 'selected')}
+ role="button"
Review Comment:
Since SelectorLabel is a button element, role is not necessary (implicitly
defined) but type is necessary for a11y POV.
```suggestion
type="button"
```
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
justinpark commented on code in PR #26602:
URL: https://github.com/apache/superset/pull/26602#discussion_r1456519542
##
superset-frontend/src/explore/components/controls/FixedOrMetricControl/index.jsx:
##
@@ -127,7 +127,7 @@ export default class FixedOrMetricControl extends
React.Component {
undefined}>
+ undefined}>
Review Comment:
Since onClick handler does nothing, both role and onClick are redundant.
```suggestion
```
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
justinpark commented on code in PR #26602:
URL: https://github.com/apache/superset/pull/26602#discussion_r1456518427
##
superset-frontend/src/explore/components/controls/CheckboxControl.jsx:
##
@@ -65,6 +65,7 @@ export default class CheckboxControl extends React.Component {
https://github.com/apache/superset/pull/26602/files#diff-1786e2046531b3067d8ed91311079f60a31e6503a457a043c92c17f29ec31adaR114
already covered
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
justinpark commented on code in PR #26602:
URL: https://github.com/apache/superset/pull/26602#discussion_r1456514988
##
superset-frontend/src/dashboard/components/filterscope/FilterScopeSelector.jsx:
##
@@ -687,6 +687,7 @@ export default class FilterScopeSelector extends
React.PureComponent {
onClick={this.onChangeFilterField}
onCheck={this.onCheckFilterField}
onExpand={this.onExpandFilterField}
+role="button"
Review Comment:
This prop is redundant since `FilterFieldTree` doesn't handle it.
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
justinpark commented on code in PR #26602:
URL: https://github.com/apache/superset/pull/26602#discussion_r1456512526
##
superset-frontend/src/dashboard/components/Header/index.jsx:
##
@@ -583,6 +584,7 @@ class Header extends React.PureComponent {
onClick={redoLength && onRedo}
data-test="redo-action"
iconSize="xl"
+ role="button"
Review Comment:
same
[issue](https://github.com/apache/superset/pull/26602/files#r1456511910) here
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
justinpark commented on code in PR #26602:
URL: https://github.com/apache/superset/pull/26602#discussion_r1456511910
##
superset-frontend/src/dashboard/components/Header/index.jsx:
##
@@ -563,6 +563,7 @@ class Header extends React.PureComponent {
onClick={undoLength && onUndo}
data-test="undo-action"
iconSize="xl"
+ role="button"
Review Comment:
Thanks @eulloa10 for all your works.
I propose moving the `onClick` event handler to the `StyledUndoRedoButton`
component itself, rather than applying a button role to the icon within it.
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
eulloa10 commented on PR #26602: URL: https://github.com/apache/superset/pull/26602#issuecomment-1895285807 Thanks @john-bodley. I resolved the conflicts, hope it's successful this time around. -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
eulloa10 closed pull request #26602: refactor: add "button" role to clickable UI elements for improved accessibility URL: https://github.com/apache/superset/pull/26602 -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
john-bodley commented on PR #26602: URL: https://github.com/apache/superset/pull/26602#issuecomment-1894238284 Thank @eulloa10 for the change. I've kicked off CI, though it seems you have a couple of conflicts you need to resolve. -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
Re: [PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
eulloa10 closed pull request #26602: refactor: add "button" role to clickable UI elements for improved accessibility URL: https://github.com/apache/superset/pull/26602 -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
[PR] refactor: add "button" role to clickable UI elements for improved accessibility [superset]
eulloa10 opened a new pull request, #26602: URL: https://github.com/apache/superset/pull/26602 ### SUMMARY Clickable UI elements lacking `` or `` tags, or the `role="button"` attribute, weren't semantically clear as clickable elements when using a screen reader. To improve accessibility, I reviewed the React codebase for `onClick` handlers. When encountering HTML elements with `onClick` handlers AND without `` or `` tags, I added the `role="button"` attribute to identify them as clickable. This approach targets the most immediate and straightforward cases, ensuring that elements triggered by onClick handlers are now clearly identified as clickable for users relying on screen readers. ### TESTING INSTRUCTIONS Testing options include: - Using a screen reader: identify and navigate through the clickable components and ensure they are announced correctly - Using browser devtools: inspect clickable elements and verify that elements without `` or `` tags have the `role="button"` attribute. ### ADDITIONAL INFORMATION - [x] Has associated issue: #26190 - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] Migration is atomic, supports rollback & is backwards-compatible - [ ] Confirm DB migration upgrade and downgrade tested - [ ] Runtime estimates and downtime expectations provided - [ ] Introduces new feature or API - [ ] Removes existing feature or API -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
