fix: add additional ui tweaks #10275

graceguo-supercat · 2020-07-09T18:40:26Z

SUMMARY

This PR is to finish extra UI tweaks based on the discussion on #10257 (which was merged without review approval accidentally)

smaller and lighter x

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

After:

TEST PLAN

CI

ADDITIONAL INFORMATION

Has associated issue: feat: minor reorder SQL Lab Tab controls #10257
Changes UI

@kenchendesign @rusackas

etr2460 · 2020-07-10T00:36:35Z

I think we're starting to move towards using svg icons instead of unicode icons; if we want a thin close icon here, maybe use this one that i'm adding in my error message PR? https://github.com/apache/incubator-superset/pull/10274/files#diff-804196da1c94e898bdc1c988354ed2d0

it looks like this (only look at the icon in the upper right), thoughts @kenchendesign?

graceguo-supercat · 2020-07-10T00:43:28Z

there are so many places in the existed code based re-use css .close.
Could you make another PR, just commit this svg, and convert all existed use-cases?

After that PR i will follow the new rule.

etr2460 · 2020-07-10T00:45:25Z

If that's the case, then can you attach screenshots of all the other places this icon changed? I thought this was added in your PR, and not used in a bunch of other places

graceguo-supercat · 2020-07-10T00:55:21Z

it seems i used a close class shipped with bootstrap?

I didn't know it's from bootstrap... just want to overwrite some default behavior :)
all other places used fa fa-close. maybe i should change to that? and later when you do convert it's easier to find all uses?

graceguo-supercat · 2020-07-10T01:01:37Z

this is how it looks if simply apply fa fa-close. way too black...i need to tweak with lighter color...

etr2460 · 2020-07-10T01:26:51Z

Hmm, interesting. Let's see what @kenchendesign prefers, and then we can go with that design. In the future I can help clean up some of the icons and replace with SVGs if we don't do it here

codecov-commenter · 2020-07-10T06:03:40Z

Codecov Report

Merging #10275 into master will decrease coverage by 0.30%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #10275      +/-   ##
==========================================
- Coverage   70.20%   69.90%   -0.31%     
==========================================
  Files         598      195     -403     
  Lines       32017    18912   -13105     
  Branches     3240        0    -3240     
==========================================
- Hits        22477    13220    -9257     
+ Misses       9435     5692    -3743     
+ Partials      105        0     -105

Flag	Coverage Δ
#cypress	`?`
#javascript	`?`
#python	`69.90% <ø> (+0.21%)`	⬆️

Impacted Files	Coverage Δ
superset/sql_parse.py	`99.30% <0.00%> (-0.01%)`	⬇️
superset/errors.py	`100.00% <0.00%> (ø)`
superset/viz_sip38.py	`0.00% <0.00%> (ø)`
superset/views/log/api.py	`100.00% <0.00%> (ø)`
superset/views/sql_lab.py	`62.57% <0.00%> (ø)`
superset/charts/schemas.py	`100.00% <0.00%> (ø)`
...ontend/src/dashboard/components/menu/HoverMenu.jsx
superset-frontend/src/welcome/App.jsx
superset-frontend/src/components/VictoryTheme.js
...components/AdhocFilterEditPopoverSqlTabContent.jsx
... and 411 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c67b1ab...21ccebd. Read the comment docs.

mistercrunch · 2020-07-10T06:35:26Z

superset-frontend/src/SqlLab/components/TabbedSqlEditors.jsx

-            {'×'}
-          </span>
+          <i
+            className="fa fa-close"


Another option is to use text-muted here.

We have grays defined somewhere I think. We should make classes for our grays that we can use anywhere...

https://github.com/apache/incubator-superset/blob/master/superset-frontend/stylesheets/less/variables.less#L48-L56

not sure how to use these grays in this context, @rusackas ?

create classes in superset.less and use them as classes

create a ./TabbedSqlEditors.less and use the variable in there?

use emotion?

it might be even better to use emotion here. The colors should all be in styled

thanks for suggestion. @rusackas do we have any example that i can follow to make similar thing? i am not sure which gray to use. thanks!

kenchendesign · 2020-07-14T00:59:15Z

@etr2460 Yeah I prefer the skinny close icon.

graceguo-supercat · 2020-07-14T23:40:51Z

Updated cancel icon by a new svg icon:

etr2460 · 2020-07-15T01:20:30Z

superset-frontend/src/SqlLab/components/TabbedSqlEditors.jsx

+            name="cancel-x"
+            height="16px"
+            width="16px"
+            viewBox="4 4 16 16"


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

etr2460 · 2020-07-15T01:20:53Z

superset-frontend/src/SqlLab/components/TabbedSqlEditors.jsx

+            height="16px"
+            width="16px"


I'm surprised the pixel strings are required here, did you test to see if numbers work?

It turn out all these extra props are not necessary...i thought new icon pushed the tab taller but actually not. new svg (with 24 x 24) is good to use.

etr2460 · 2020-07-15T01:30:15Z

superset-frontend/src/SqlLab/main.less

@@ -259,6 +259,22 @@ div.Workspace {
    &:active {
      background: none;
    }
+
+    .fa.fa-close {


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:

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: 80b06f6#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 Icons that are clickable. @nytai @lilykuang or @rusackas, would any of you be interested in this? Or should I take it on?

since when a11y is a requirement?

not a requirement, but certainly something we should consider i think! I can't even imagine how bad it is to use Superset with a screenreader or keyboard controls....

correct. this PR can't handle big issue like that. no one will care there is an icon in whole Superset is a11y. yay!!

this was introduced by last try (to use font awesome icon). fixed

could you at least add role="button" tabIndex={0} to the Icon component? That should be sufficient here

I can do change for this place. but you probably have to fix all other places...

yup, i'll fix other stuff in the future, likely by adding a lint rule

graceguo-supercat · 2020-07-15T17:57:58Z

superset-frontend/src/SqlLab/main.less

@@ -177,11 +177,9 @@ div.Workspace {

  display: inline-block;
  background-color: @gray-light;
-  line-height: 8px; // set specifically for closing 'x'


Have to remove these 2 lines to make circle and new svg x icon aligned.

etr2460 · 2020-07-15T18:39:17Z

superset-frontend/src/SqlLab/main.less

+    svg {
+      vertical-align: middle;
+
+      &:hover {
+        cursor: pointer;
+      }
+    }


Instead of this, you can add the styles in JS as follows:

const CloseIcon = styled(Icon)` vertical-align: middle; &:hover { cursor: pointer; } `; // then inside the render function do <CloseIcon role="button" tabIndex={0} name="cancel-x" onClick={() => this.removeQueryEditor(qe)} />

If every time use our own svg need this much trouble, i would rather use Font-Awesome, they took care of these common behaviors (like role="button" tabIndex={0}) already.

I think you can just add a prop to the Icon component and extend the style there.

Actually, you don't have to do anything. Just apply cursor directly to CloseIcon since it's a valid attribute for SVG elements. <CloseIcon role="button" cursor="pointer">.

You also don't have to &:hover for cursor attributes, since it already only applies when an element is hovered.

but I still need vertical-align: middle;

Yeah, I think it makes sense to keep that rule in main.less for now. We'll see if it's necessary to generalize it to Icon.tsx when we see it being needed in more places.

ok, i will follow this suggestion :)

etr2460

lgtm after you remove the unused import. thanks for the iteration to get this aligned with the newest in Superset UI architecture!

etr2460 · 2020-07-15T20:40:29Z

superset-frontend/src/SqlLab/components/TabbedSqlEditors.jsx

@@ -24,11 +24,13 @@ import { bindActionCreators } from 'redux';
 import URI from 'urijs';
 import { t } from '@superset-ui/translation';
 import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags';
+import styled from '@superset-ui/style';


seems like you can remove this import now

* fix: add additional ui tweaks * use fa fa-close class for x icon * use new svg icons

fix: add additional ui tweaks

1f27fee

pull-request-size bot added the size/XS label Jul 9, 2020

pull-request-size bot added size/S and removed size/XS labels Jul 10, 2020

use fa fa-close class for x icon

7a030d3

graceguo-supercat force-pushed the gg-FixTabControlsUI branch from e443728 to 7a030d3 Compare July 10, 2020 05:53

mistercrunch reviewed Jul 10, 2020

View reviewed changes

pull-request-size bot added size/M and removed size/S labels Jul 14, 2020

etr2460 reviewed Jul 15, 2020

View reviewed changes

graceguo-supercat force-pushed the gg-FixTabControlsUI branch from 7880cd6 to aba7c11 Compare July 15, 2020 17:53

pull-request-size bot added size/S and removed size/M labels Jul 15, 2020

graceguo-supercat commented Jul 15, 2020

View reviewed changes

etr2460 reviewed Jul 15, 2020

View reviewed changes

graceguo-supercat force-pushed the gg-FixTabControlsUI branch from aba7c11 to ff2c1aa Compare July 15, 2020 19:04

pull-request-size bot added size/M and removed size/S labels Jul 15, 2020

graceguo-supercat force-pushed the gg-FixTabControlsUI branch from ff2c1aa to 793681e Compare July 15, 2020 19:43

pull-request-size bot added size/S and removed size/M labels Jul 15, 2020

graceguo-supercat force-pushed the gg-FixTabControlsUI branch from 793681e to f9d7590 Compare July 15, 2020 20:35

etr2460 approved these changes Jul 15, 2020

View reviewed changes

use new svg icons

21ccebd

graceguo-supercat force-pushed the gg-FixTabControlsUI branch from f9d7590 to 21ccebd Compare July 15, 2020 20:45

graceguo-supercat merged commit 2f147cf into apache:master Jul 15, 2020

etr2460 mentioned this pull request Jul 17, 2020

chore: enable no-static-element-interactions eslint rule #10351

Merged

6 tasks

graceguo-supercat deleted the gg-FixTabControlsUI branch August 27, 2020 20:32

auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020

fix: add additional ui tweaks (apache#10275)

6f222e0

* fix: add additional ui tweaks * use fa fa-close class for x icon * use new svg icons

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.38.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add additional ui tweaks #10275

fix: add additional ui tweaks #10275

graceguo-supercat commented Jul 9, 2020 •

edited

Loading

etr2460 commented Jul 10, 2020 •

edited

Loading

graceguo-supercat commented Jul 10, 2020 •

edited

Loading

etr2460 commented Jul 10, 2020 •

edited

Loading

graceguo-supercat commented Jul 10, 2020 •

edited

Loading

graceguo-supercat commented Jul 10, 2020 •

edited

Loading

etr2460 commented Jul 10, 2020

codecov-commenter commented Jul 10, 2020 •

edited

Loading

mistercrunch Jul 10, 2020

mistercrunch Jul 10, 2020

etr2460 Jul 10, 2020

graceguo-supercat Jul 13, 2020

kenchendesign commented Jul 14, 2020

graceguo-supercat commented Jul 14, 2020 •

edited

Loading

etr2460 Jul 15, 2020

etr2460 Jul 15, 2020

graceguo-supercat Jul 15, 2020 •

edited

Loading

etr2460 Jul 15, 2020

graceguo-supercat Jul 15, 2020

etr2460 Jul 15, 2020

graceguo-supercat Jul 15, 2020

graceguo-supercat Jul 15, 2020 •

edited

Loading

etr2460 Jul 15, 2020

graceguo-supercat Jul 15, 2020

etr2460 Jul 15, 2020

graceguo-supercat Jul 15, 2020

etr2460 Jul 15, 2020

graceguo-supercat Jul 15, 2020 •

edited

Loading

ktmud Jul 15, 2020

ktmud Jul 15, 2020

graceguo-supercat Jul 15, 2020

ktmud Jul 15, 2020

graceguo-supercat Jul 15, 2020

etr2460 left a comment

etr2460 Jul 15, 2020

fix: add additional ui tweaks #10275

fix: add additional ui tweaks #10275

Conversation

graceguo-supercat commented Jul 9, 2020 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

etr2460 commented Jul 10, 2020 • edited Loading

graceguo-supercat commented Jul 10, 2020 • edited Loading

etr2460 commented Jul 10, 2020 • edited Loading

graceguo-supercat commented Jul 10, 2020 • edited Loading

graceguo-supercat commented Jul 10, 2020 • edited Loading

etr2460 commented Jul 10, 2020

codecov-commenter commented Jul 10, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenchendesign commented Jul 14, 2020

graceguo-supercat commented Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graceguo-supercat Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graceguo-supercat Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graceguo-supercat Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etr2460 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graceguo-supercat commented Jul 9, 2020 •

edited

Loading

etr2460 commented Jul 10, 2020 •

edited

Loading

graceguo-supercat commented Jul 10, 2020 •

edited

Loading

etr2460 commented Jul 10, 2020 •

edited

Loading

graceguo-supercat commented Jul 10, 2020 •

edited

Loading

graceguo-supercat commented Jul 10, 2020 •

edited

Loading

codecov-commenter commented Jul 10, 2020 •

edited

Loading

graceguo-supercat commented Jul 14, 2020 •

edited

Loading

graceguo-supercat Jul 15, 2020 •

edited

Loading

graceguo-supercat Jul 15, 2020 •

edited

Loading

graceguo-supercat Jul 15, 2020 •

edited

Loading