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(<MetadataBar items={items} />);
+
+ 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(<MetadataBar items={items} />);
+ 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(<MetadataBar items={items} />);
+
+ 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 (
<Tag
onClick={onClick}
+ role={onClick ? 'button' : undefined}
Review Comment:
Ok I see this `Tag` is pulled directly from Antdesign rather than our custom
`Tag` component, so it makes sense to keep it here
##########
superset-frontend/src/explore/components/controls/FixedOrMetricControl/index.jsx:
##########
@@ -127,7 +127,7 @@ export default class FixedOrMetricControl extends
React.Component {
<Collapse.Panel
showArrow={false}
header={
- <Label onClick={() => 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
