You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/07/09 18:40:33 UTC

[GitHub] [incubator-superset] graceguo-supercat opened a new pull request #10275: fix: add additional ui tweaks

graceguo-supercat opened a new pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275


   ### 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:**
   <img width="776" alt="Screen Shot 2020-07-09 at 11 28 04 AM" src="https://user-images.githubusercontent.com/27990562/87077746-764bd780-c1d8-11ea-87f7-926542bda8f5.png">
   
   **After:**
   <img width="682" alt="Screen Shot 2020-07-09 at 11 31 48 AM" src="https://user-images.githubusercontent.com/27990562/87077756-7a77f500-c1d8-11ea-982c-6206168e5a4d.png">
   
   
   ### TEST PLAN
   CI
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [x] Has associated issue: #10257 
   - [ ] Changes UI
   - [ ] Requires DB Migration.
   - [ ] Confirm DB Migration upgrade and downgrade tested.
   - [ ] 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat commented on a change in pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#discussion_r455233531



##########
File path: superset-frontend/src/SqlLab/main.less
##########
@@ -259,6 +259,22 @@ div.Workspace {
     &:active {
       background: none;
     }
+
+    .fa.fa-close {

Review comment:
       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 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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] mistercrunch commented on a change in pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
mistercrunch commented on a change in pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#discussion_r452650000



##########
File path: superset-frontend/src/SqlLab/components/TabbedSqlEditors.jsx
##########
@@ -289,9 +289,10 @@ class TabbedSqlEditors extends React.PureComponent {
       const title = (
         <>
           {qe.title} <TabStatusIcon tabState={state} />{' '}
-          <span className="close" onClick={() => this.removeQueryEditor(qe)}>
-            {'×'}
-          </span>
+          <i
+            className="fa fa-close"

Review comment:
       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?




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat commented on a change in pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#discussion_r455186286



##########
File path: superset-frontend/src/SqlLab/main.less
##########
@@ -259,6 +259,22 @@ div.Workspace {
     &:active {
       background: none;
     }
+
+    .fa.fa-close {

Review comment:
       since when a11y is a requirement?




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] codecov-commenter edited a comment on pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#issuecomment-656500471






----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat edited a comment on pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#issuecomment-656422291


   it seems i used a `close` class shipped with bootstrap?
   
   <img width="1306" alt="Screen Shot 2020-07-09 at 5 50 04 PM" src="https://user-images.githubusercontent.com/27990562/87104455-d52b4400-c20c-11ea-969d-3081ca18bca9.png">
   
   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?
   


----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] etr2460 commented on a change in pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#discussion_r455218040



##########
File path: superset-frontend/src/SqlLab/main.less
##########
@@ -259,6 +259,22 @@ div.Workspace {
     &:active {
       background: none;
     }
+
+    .fa.fa-close {

Review comment:
       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....




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] ktmud commented on a change in pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#discussion_r455305516



##########
File path: superset-frontend/src/SqlLab/main.less
##########
@@ -259,6 +257,14 @@ div.Workspace {
     &:active {
       background: none;
     }
+
+    svg {
+      vertical-align: middle;
+
+      &:hover {
+        cursor: pointer;
+      }
+    }

Review comment:
       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.




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat commented on a change in pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#discussion_r455232607



##########
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:
       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.




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] etr2460 commented on a change in pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#discussion_r455311638



##########
File path: superset-frontend/src/SqlLab/main.less
##########
@@ -259,6 +259,22 @@ div.Workspace {
     &:active {
       background: none;
     }
+
+    .fa.fa-close {

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




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat commented on a change in pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#discussion_r453795378



##########
File path: superset-frontend/src/SqlLab/components/TabbedSqlEditors.jsx
##########
@@ -289,9 +289,10 @@ class TabbedSqlEditors extends React.PureComponent {
       const title = (
         <>
           {qe.title} <TabStatusIcon tabState={state} />{' '}
-          <span className="close" onClick={() => this.removeQueryEditor(qe)}>
-            {'×'}
-          </span>
+          <i
+            className="fa fa-close"

Review comment:
       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!




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] etr2460 commented on pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
etr2460 commented on pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#issuecomment-656430443


   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


----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] codecov-commenter edited a comment on pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#issuecomment-656500471


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10275?src=pr&el=h1) Report
   > Merging [#10275](https://codecov.io/gh/apache/incubator-superset/pull/10275?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/c67b1abfd8bd79aadb529cd58530f1e11eb12a65&el=desc) will **decrease** coverage by `0.08%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/10275/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/10275?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #10275      +/-   ##
   ==========================================
   - Coverage   70.20%   70.11%   -0.09%     
   ==========================================
     Files         598      598              
     Lines       32017    32002      -15     
     Branches     3240     3234       -6     
   ==========================================
   - Hits        22477    22438      -39     
   - Misses       9435     9456      +21     
   - Partials      105      108       +3     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #cypress | `52.94% <100.00%> (-0.52%)` | :arrow_down: |
   | #javascript | `59.40% <0.00%> (+0.06%)` | :arrow_up: |
   | #python | `69.68% <ø> (ø)` | |
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10275?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...rontend/src/SqlLab/components/TabbedSqlEditors.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10275/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RhYmJlZFNxbEVkaXRvcnMuanN4) | `77.27% <100.00%> (-5.20%)` | :arrow_down: |
   | [...et-frontend/src/SqlLab/reducers/getInitialState.js](https://codecov.io/gh/apache/incubator-superset/pull/10275/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9yZWR1Y2Vycy9nZXRJbml0aWFsU3RhdGUuanM=) | `33.33% <0.00%> (-16.67%)` | :arrow_down: |
   | [...rontend/src/SqlLab/components/QueryAutoRefresh.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10275/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1F1ZXJ5QXV0b1JlZnJlc2guanN4) | `65.90% <0.00%> (-6.82%)` | :arrow_down: |
   | [...rontend/src/SqlLab/components/SqlEditorLeftBar.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10275/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NxbEVkaXRvckxlZnRCYXIuanN4) | `44.00% <0.00%> (-4.00%)` | :arrow_down: |
   | [...end/src/SqlLab/components/TemplateParamsEditor.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10275/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RlbXBsYXRlUGFyYW1zRWRpdG9yLmpzeA==) | `88.57% <0.00%> (-2.86%)` | :arrow_down: |
   | [superset-frontend/src/SqlLab/actions/sqlLab.js](https://codecov.io/gh/apache/incubator-superset/pull/10275/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9hY3Rpb25zL3NxbExhYi5qcw==) | `60.25% <0.00%> (-2.36%)` | :arrow_down: |
   | [superset-frontend/src/SqlLab/reducers/sqlLab.js](https://codecov.io/gh/apache/incubator-superset/pull/10275/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9yZWR1Y2Vycy9zcWxMYWIuanM=) | `37.75% <0.00%> (-1.66%)` | :arrow_down: |
   | [...erset-frontend/src/SqlLab/components/SqlEditor.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10275/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1NxbEVkaXRvci5qc3g=) | `52.12% <0.00%> (-1.22%)` | :arrow_down: |
   | [...rontend/src/SqlLab/components/AceEditorWrapper.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10275/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL0FjZUVkaXRvcldyYXBwZXIudHN4) | `55.91% <0.00%> (-1.08%)` | :arrow_down: |
   | ... and [1 more](https://codecov.io/gh/apache/incubator-superset/pull/10275/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10275?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10275?src=pr&el=footer). Last update [c67b1ab...7a030d3](https://codecov.io/gh/apache/incubator-superset/pull/10275?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat commented on a change in pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#discussion_r455267497



##########
File path: superset-frontend/src/SqlLab/main.less
##########
@@ -259,6 +259,22 @@ div.Workspace {
     &:active {
       background: none;
     }
+
+    .fa.fa-close {

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




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] etr2460 commented on a change in pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#discussion_r455262965



##########
File path: superset-frontend/src/SqlLab/main.less
##########
@@ -259,6 +257,14 @@ div.Workspace {
     &:active {
       background: none;
     }
+
+    svg {
+      vertical-align: middle;
+
+      &:hover {
+        cursor: pointer;
+      }
+    }

Review comment:
       Instead of this, you can add the styles in JS as follows:
   ```tsx
   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)} />
   ```




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat commented on pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#issuecomment-656423758


   <img width="734" alt="Screen Shot 2020-07-09 at 6 00 03 PM" src="https://user-images.githubusercontent.com/27990562/87104871-153ef680-c20e-11ea-8a48-e4a104dffdc4.png">
   
   this is how it looks if use `fa fa-close` class. way too black...i need to tweak with lighter color...


----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] ktmud commented on a change in pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#discussion_r455280523



##########
File path: superset-frontend/src/SqlLab/main.less
##########
@@ -259,6 +257,14 @@ div.Workspace {
     &:active {
       background: none;
     }
+
+    svg {
+      vertical-align: middle;
+
+      &:hover {
+        cursor: pointer;
+      }
+    }

Review comment:
       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.




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat commented on a change in pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#discussion_r455268601



##########
File path: superset-frontend/src/SqlLab/main.less
##########
@@ -259,6 +257,14 @@ div.Workspace {
     &:active {
       background: none;
     }
+
+    svg {
+      vertical-align: middle;
+
+      &:hover {
+        cursor: pointer;
+      }
+    }

Review comment:
       If every time use our own svg need this much trouble, i would rather use Font-Awesome, they took care of these troubles already




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat commented on pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#issuecomment-658465262


   Updated cancel icon with new svg icon:
   <img width="758" alt="Screen Shot 2020-07-14 at 4 16 07 PM" src="https://user-images.githubusercontent.com/27990562/87486695-b522c880-c5f0-11ea-99a8-0313e9a43b42.png">
   


----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat edited a comment on pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#issuecomment-658465262


   Updated cancel icon by a new svg icon:
   <img width="758" alt="Screen Shot 2020-07-14 at 4 16 07 PM" src="https://user-images.githubusercontent.com/27990562/87486695-b522c880-c5f0-11ea-99a8-0313e9a43b42.png">
   


----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat commented on pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#issuecomment-656422291


   it seems i used css class shipped with bootstrap?
   <img width="1306" alt="Screen Shot 2020-07-09 at 5 50 04 PM" src="https://user-images.githubusercontent.com/27990562/87104455-d52b4400-c20c-11ea-969d-3081ca18bca9.png">
   
   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?
   


----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat commented on a change in pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#discussion_r455268601



##########
File path: superset-frontend/src/SqlLab/main.less
##########
@@ -259,6 +257,14 @@ div.Workspace {
     &:active {
       background: none;
     }
+
+    svg {
+      vertical-align: middle;
+
+      &:hover {
+        cursor: pointer;
+      }
+    }

Review comment:
       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.




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat commented on a change in pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#discussion_r455232607



##########
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:
       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 14) is good to use.




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] etr2460 commented on a change in pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] etr2460 commented on a change in pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#discussion_r452933460



##########
File path: superset-frontend/src/SqlLab/components/TabbedSqlEditors.jsx
##########
@@ -289,9 +289,10 @@ class TabbedSqlEditors extends React.PureComponent {
       const title = (
         <>
           {qe.title} <TabStatusIcon tabState={state} />{' '}
-          <span className="close" onClick={() => this.removeQueryEditor(qe)}>
-            {'×'}
-          </span>
+          <i
+            className="fa fa-close"

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




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat commented on a change in pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#discussion_r455236610



##########
File path: superset-frontend/src/SqlLab/main.less
##########
@@ -259,6 +259,22 @@ div.Workspace {
     &:active {
       background: none;
     }
+
+    .fa.fa-close {

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




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] etr2460 commented on a change in pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#discussion_r455328616



##########
File path: 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';

Review comment:
       seems like you can remove this import now




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat commented on a change in pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#discussion_r455235862



##########
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:
       yes. this was introduced by last try (to use font awsome icon), should be removed.




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat commented on pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#issuecomment-656419634


   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?


----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] etr2460 commented on a change in pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#discussion_r455261608



##########
File path: superset-frontend/src/SqlLab/main.less
##########
@@ -259,6 +259,22 @@ div.Workspace {
     &:active {
       background: none;
     }
+
+    .fa.fa-close {

Review comment:
       could you at least add `role="button" tabIndex={0}` to the `Icon` component? That should be sufficient 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] ktmud commented on a change in pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#discussion_r455278396



##########
File path: superset-frontend/src/SqlLab/main.less
##########
@@ -259,6 +257,14 @@ div.Workspace {
     &:active {
       background: none;
     }
+
+    svg {
+      vertical-align: middle;
+
+      &:hover {
+        cursor: pointer;
+      }
+    }

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




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] etr2460 edited a comment on pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
etr2460 edited a comment on pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#issuecomment-656420108


   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


----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] kenchendesign commented on pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
kenchendesign commented on pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#issuecomment-657905345


   @etr2460 Yeah I prefer the skinny close icon.


----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat merged pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
graceguo-supercat merged pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275


   


----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] etr2460 commented on pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
etr2460 commented on pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#issuecomment-656418060


   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
   
   


----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat commented on a change in pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#discussion_r455318994



##########
File path: superset-frontend/src/SqlLab/main.less
##########
@@ -259,6 +257,14 @@ div.Workspace {
     &:active {
       background: none;
     }
+
+    svg {
+      vertical-align: middle;
+
+      &:hover {
+        cursor: pointer;
+      }
+    }

Review comment:
       ok, i will follow this 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat edited a comment on pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#issuecomment-656419634


   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.


----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat commented on a change in pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#discussion_r455237584



##########
File path: 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'

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




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat edited a comment on pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#issuecomment-656423758


   <img width="734" alt="Screen Shot 2020-07-09 at 6 00 03 PM" src="https://user-images.githubusercontent.com/27990562/87104871-153ef680-c20e-11ea-8a48-e4a104dffdc4.png">
   
   this is how it looks if simply apply `fa fa-close`. way too black...i need to tweak with lighter color...


----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat commented on a change in pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#discussion_r455297087



##########
File path: superset-frontend/src/SqlLab/main.less
##########
@@ -259,6 +257,14 @@ div.Workspace {
     &:active {
       background: none;
     }
+
+    svg {
+      vertical-align: middle;
+
+      &:hover {
+        cursor: pointer;
+      }
+    }

Review comment:
       but I still need `vertical-align: middle;` 




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] mistercrunch commented on a change in pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
mistercrunch commented on a change in pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#discussion_r452648987



##########
File path: superset-frontend/src/SqlLab/components/TabbedSqlEditors.jsx
##########
@@ -289,9 +289,10 @@ class TabbedSqlEditors extends React.PureComponent {
       const title = (
         <>
           {qe.title} <TabStatusIcon tabState={state} />{' '}
-          <span className="close" onClick={() => this.removeQueryEditor(qe)}>
-            {'×'}
-          </span>
+          <i
+            className="fa fa-close"

Review comment:
       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...




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] codecov-commenter commented on pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#issuecomment-656500471


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10275?src=pr&el=h1) Report
   > Merging [#10275](https://codecov.io/gh/apache/incubator-superset/pull/10275?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/c67b1abfd8bd79aadb529cd58530f1e11eb12a65&el=desc) will **decrease** coverage by `4.73%`.
   > The diff coverage is `0.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/10275/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/10275?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #10275      +/-   ##
   ==========================================
   - Coverage   70.20%   65.46%   -4.74%     
   ==========================================
     Files         598      598              
     Lines       32017    32001      -16     
     Branches     3240     3234       -6     
   ==========================================
   - Hits        22477    20949    -1528     
   - Misses       9435    10871    +1436     
   - Partials      105      181      +76     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #cypress | `?` | |
   | #javascript | `59.40% <0.00%> (+0.06%)` | :arrow_up: |
   | #python | `69.68% <ø> (ø)` | |
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10275?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...rontend/src/SqlLab/components/TabbedSqlEditors.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10275/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RhYmJlZFNxbEVkaXRvcnMuanN4) | `74.02% <0.00%> (-8.45%)` | :arrow_down: |
   | [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10275/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/explore/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10275/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvQXBwLmpzeA==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/SqlLab/index.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10275/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9pbmRleC5qc3g=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/dashboard/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10275/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/explore/index.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10275/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvaW5kZXguanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/dashboard/index.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10275/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9pbmRleC5qc3g=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/setup/setupColors.js](https://codecov.io/gh/apache/incubator-superset/pull/10275/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwQ29sb3JzLmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/chart/ChartContainer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10275/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0Q29udGFpbmVyLmpzeA==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/setup/setupFormatters.js](https://codecov.io/gh/apache/incubator-superset/pull/10275/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwRm9ybWF0dGVycy5qcw==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | ... and [139 more](https://codecov.io/gh/apache/incubator-superset/pull/10275/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10275?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10275?src=pr&el=footer). Last update [c67b1ab...7a030d3](https://codecov.io/gh/apache/incubator-superset/pull/10275?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] etr2460 edited a comment on pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
etr2460 edited a comment on pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#issuecomment-656418060


   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, thoughts @kenchendesign? 
   ![image](https://user-images.githubusercontent.com/7409244/87103794-d9566200-c20a-11ea-9ac2-1fabb0432825.png)
   


----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat commented on a change in pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#discussion_r455268601



##########
File path: superset-frontend/src/SqlLab/main.less
##########
@@ -259,6 +257,14 @@ div.Workspace {
     &:active {
       background: none;
     }
+
+    svg {
+      vertical-align: middle;
+
+      &:hover {
+        cursor: pointer;
+      }
+    }

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




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] graceguo-supercat commented on a change in pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#discussion_r455236610



##########
File path: superset-frontend/src/SqlLab/main.less
##########
@@ -259,6 +259,22 @@ div.Workspace {
     &:active {
       background: none;
     }
+
+    .fa.fa-close {

Review comment:
       this was introduced by last try (to use font awsome icon). fixed




----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] etr2460 commented on pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
etr2460 commented on pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#issuecomment-656420108


   If that's the case, then can you attach screenshots of all the other places this icon changed?


----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] codecov-commenter edited a comment on pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#issuecomment-656500471


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10275?src=pr&el=h1) Report
   > Merging [#10275](https://codecov.io/gh/apache/incubator-superset/pull/10275?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/c67b1abfd8bd79aadb529cd58530f1e11eb12a65&el=desc) will **increase** coverage by `0.02%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/10275/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/10275?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #10275      +/-   ##
   ==========================================
   + Coverage   70.20%   70.22%   +0.02%     
   ==========================================
     Files         598      598              
     Lines       32017    32002      -15     
     Branches     3240     3234       -6     
   ==========================================
   - Hits        22477    22473       -4     
   + Misses       9435     9424      -11     
     Partials      105      105              
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #cypress | `53.45% <100.00%> (-0.01%)` | :arrow_down: |
   | #javascript | `59.40% <0.00%> (+0.06%)` | :arrow_up: |
   | #python | `69.68% <ø> (ø)` | |
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/10275?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...rontend/src/SqlLab/components/TabbedSqlEditors.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10275/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1RhYmJlZFNxbEVkaXRvcnMuanN4) | `82.46% <100.00%> (ø)` | |
   | [...rontend/src/SqlLab/components/QueryAutoRefresh.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10275/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1F1ZXJ5QXV0b1JlZnJlc2guanN4) | `65.90% <0.00%> (-6.82%)` | :arrow_down: |
   | [superset-frontend/src/SqlLab/actions/sqlLab.js](https://codecov.io/gh/apache/incubator-superset/pull/10275/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9hY3Rpb25zL3NxbExhYi5qcw==) | `62.60% <0.00%> (ø)` | |
   | [...ontend/src/explore/controlPanels/Shared\_DeckGL.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10275/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29udHJvbFBhbmVscy9TaGFyZWRfRGVja0dMLmpzeA==) | `82.05% <0.00%> (+20.94%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10275?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10275?src=pr&el=footer). Last update [c67b1ab...7a030d3](https://codecov.io/gh/apache/incubator-superset/pull/10275?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [incubator-superset] etr2460 edited a comment on pull request #10275: fix: add additional ui tweaks

Posted by GitBox <gi...@apache.org>.
etr2460 edited a comment on pull request #10275:
URL: https://github.com/apache/incubator-superset/pull/10275#issuecomment-656418060


   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? 
   ![image](https://user-images.githubusercontent.com/7409244/87103794-d9566200-c20a-11ea-9ac2-1fabb0432825.png)
   


----------------------------------------------------------------
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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org