etr2460 commented on a change in pull request #10275:
URL:
https://github.com/apache/incubator-superset/pull/10275#discussion_r454734453
##########
File path: superset-frontend/src/SqlLab/components/TabbedSqlEditors.jsx
##########
@@ -289,9 +290,13 @@ class TabbedSqlEditors extends React.PureComponent {
const title = (
<>
{qe.title} <TabStatusIcon tabState={state} />{' '}
- <span className="close" onClick={() => this.removeQueryEditor(qe)}>
- {'×'}
- </span>
+ <Icon
+ name="cancel-x"
+ height="16px"
+ width="16px"
Review comment:
I'm surprised the pixel strings are required here, did you test to see
if numbers work?
##########
File path: superset-frontend/src/SqlLab/main.less
##########
@@ -259,6 +259,22 @@ div.Workspace {
&:active {
background: none;
}
+
+ .fa.fa-close {
Review comment:
I don't see the font awesome class applied anywhere, does this actually
do something?
If you need to add styling to the Icon, I'd recommend making a oneoff
element in the `TabbedSqlEditors` file like so:
```typescript
const CloseIcon = styled(Icon)`
// add css styles here
.or-add-a-class {
// more styles
}
`;
```
However, it looks like this styling is specifically to make the Icon
interactable. In that case, to ensure a11y, you should wrap it in an `a` tag, a
`button` tag, or some other element with the appropriate aria role applied,
similar to here:
https://github.com/apache/incubator-superset/commit/80b06f682727338fc6ce7e804466620cffbcc13c#diff-ce4d2524d6740fe3a5ac128b5e33baa4R382
On a side note, we should create an `IconButton` component that abstracts
all this a11y stuff away and replaces the current uses of bare `Icon`s that are
clickable. @nytai @lilykuang or @rusackas, would any of you be interested in
this? Or should I take it on?
##########
File path: superset-frontend/src/SqlLab/components/TabbedSqlEditors.jsx
##########
@@ -289,9 +290,13 @@ class TabbedSqlEditors extends React.PureComponent {
const title = (
<>
{qe.title} <TabStatusIcon tabState={state} />{' '}
- <span className="close" onClick={() => this.removeQueryEditor(qe)}>
- {'×'}
- </span>
+ <Icon
+ name="cancel-x"
+ height="16px"
+ width="16px"
+ viewBox="4 4 16 16"
Review comment:
is this required because the viewbox wasn't set correctly on the
original icon? if so, maybe we should change the SVG, since it's not used
anywhere else yet
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
