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) => {
<Fade
data-test="fade"
hovered={hovered}
+ role="button"
Review Comment:
Let's verify whether this Fade component might be useful for some other
parts of the application. If so we can move it in src/components, if not I
think we can keep it as it is
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.jsx:
##########
@@ -431,6 +431,7 @@ export class TableRenderer extends React.Component {
key={`colKey-${flatColKey}`}
colSpan={colSpan}
rowSpan={rowSpan}
+ role="columnheader button"
Review Comment:
I think we can keep it as it is considering this is a plugin. If we see the
need of using this in multiple plugins, we might consider the idea of creating
a base component specifically available to the plugins
##########
superset-frontend/src/components/Label/index.tsx:
##########
@@ -92,6 +92,7 @@ export default function Label(props: LabelProps) {
return (
<Tag
+ role="button"
Review Comment:
We could apply the same logic of checking for an onClick prop on the Tag
component too
##########
superset-frontend/src/components/Table/cell-renderers/ActionCell/index.tsx:
##########
@@ -103,7 +103,7 @@ function ActionMenu(props: ActionMenuProps) {
};
return (
- <StyledMenu onClick={handleClick}>
+ <StyledMenu role="button" onClick={handleClick}>
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 (
<Menu>
{[...new Set(limitDropdown)].map(limit => (
- <Menu.Item key={`${limit}`} onClick={() => setQueryLimit(limit)}>
+ <Menu.Item
Review Comment:
We can modify the Menu.Item itself that we are exporting from the
src/components to conditionally verify what role will be more appropriate
##########
superset-frontend/src/components/PopoverDropdown/index.tsx:
##########
@@ -93,7 +93,10 @@ const PopoverDropdown = (props: PopoverDropdownProps) => {
trigger={['click']}
overlayStyle={{ zIndex: theme.zIndex.max }}
overlay={
- <Menu onClick={({ key }: HandleSelectProps) => onChange(key)}>
+ <Menu
Review Comment:
I believe the MenuItem should get the button role but not the Menu itself
##########
superset-frontend/src/components/CachedLabel/index.tsx:
##########
@@ -44,6 +44,7 @@ const CacheLabel: React.FC<CacheLabelProps> = ({
<Label
className={`${className}`}
type={labelType}
+ role="button"
Review Comment:
Agreed
##########
superset-frontend/src/dashboard/components/CssEditor/index.jsx:
##########
@@ -82,7 +82,7 @@ class CssEditor extends React.PureComponent {
renderTemplateSelector() {
if (this.props.templates) {
const menu = (
- <Menu onClick={this.changeCssTemplate}>
+ <Menu role="button" onClick={this.changeCssTemplate}>
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 <Icons.SortAsc onClick={this.sortColumn(col, 2)} />;
+ return <Icons.SortAsc role="button" onClick={this.sortColumn(col, 2)} />;
}
if (this.state.sortColumn === col && this.state.sort === SortOrder.desc) {
- return <Icons.SortDesc onClick={this.sortColumn(col, 0)} />;
+ return <Icons.SortDesc role="button" onClick={this.sortColumn(col, 0)}
/>;
}
- return <Icons.Sort onClick={this.sortColumn(col, 1)} />;
+ return <Icons.Sort role="button" onClick={this.sortColumn(col, 1)} />;
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<IndicatorProps> = ({
const resultValue = getFilterValueForDisplay(value);
return (
<FilterItem
+ role="button"
Review Comment:
Similar to other comments, the button role could probably be added
conditionally
--
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]
