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/12/11 12:23:23 UTC

[GitHub] [incubator-superset] michael-s-molina opened a new pull request #12010: Fix anchor-is-valid lint warnings

michael-s-molina opened a new pull request #12010:
URL: https://github.com/apache/incubator-superset/pull/12010


   ### SUMMARY
   Removes all [anchor-is-valid](https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/blob/master/docs/rules/anchor-is-valid.md) lint warnings.
   
   All anchor tags that were being used as a button and not making use of `href` were replaced by a similar component. A component called `Link` were always being used as an icon button and no valid `href` so I renamed to `IconTooltip`. We already have a `Link` component in `react-router-dom`.
   
   Another benefit of this change is that the icon buttons now have their colors provided by Emotion using the theme. I noticed that some of the icons are coming from images/icons and some are coming from bootstrap classes. When we unify all this we can create an icon property in IconTooltip of a type available in images/icons. For now I kept the use of class names for icon definition.
   
   I revised all the interfaces affected by this change to make sure the interaction remained the same. There were some minor changes like no underscore in icon buttons and lighter base/hover colors that came from the theme.
   
   ### ADDITIONAL INFORMATION
   - [ ] Has associated issue:
   - [x] 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] rusackas merged pull request #12010: refactor: Fix anchor-is-valid lint warnings

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


   


----------------------------------------------------------------
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] rusackas commented on pull request #12010: refactor: Fix anchor-is-valid lint warnings

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


   @michael-s-molina This needs a rebase, but I think it's otherwise good to go.


----------------------------------------------------------------
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] rusackas commented on a change in pull request #12010: refactor: Fix anchor-is-valid lint warnings

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



##########
File path: superset-frontend/spec/javascripts/sqllab/IconTooltip_spec.jsx
##########
@@ -18,22 +18,24 @@
  */
 import React from 'react';
 import { shallow } from 'enzyme';
+import { Tooltip } from 'src/common/components/Tooltip';
+import IconTooltip from 'src/components/IconTooltip';
 
-import Link from 'src/components/Link';
-
-describe('Link', () => {
+describe('IconTooltip', () => {
   const mockedProps = {
     tooltip: 'This is a tooltip',
     href: 'https://www.airbnb.com',

Review comment:
       Right, since IconTooltip is a span now. I was just trying to make sure we weren't breaking any existing `Link` use cases in the codebase that might have an `href` 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] michael-s-molina commented on a change in pull request #12010: refactor: Fix anchor-is-valid lint warnings

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #12010:
URL: https://github.com/apache/incubator-superset/pull/12010#discussion_r541221006



##########
File path: superset-frontend/src/components/IconTooltip.tsx
##########
@@ -69,11 +74,11 @@ const Link = ({
         mouseEnterDelay={0.3}
         mouseLeaveDelay={0.15}
       >
-        {link}
+        {iconTooltip}
       </Tooltip>
     );
   }
-  return link;
+  return iconTooltip;
 };
 
-export default Link;
+export default IconTooltip;

Review comment:
       I also prefer named exports lol. I used default exports because of the current code style but I think we can use the same approach as typescript and use named exports for new components. Since this is a new component I'll make the change =D




----------------------------------------------------------------
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] rusackas commented on a change in pull request #12010: refactor: Fix anchor-is-valid lint warnings

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



##########
File path: superset-frontend/spec/javascripts/sqllab/TableElement_spec.jsx
##########
@@ -95,7 +95,7 @@ describe('TableElement', () => {
       },
     );
     expect(mockedActions.collapseTable.called).toBe(false);
-    wrapper.find('.table-name').simulate('click');
+    wrapper.find({ dataTest: 'collapse' }).simulate('click');

Review comment:
       or perhaps
   ```suggestion
       wrapper.find('[data-test="collapse"]').simulate('click');
   ```




----------------------------------------------------------------
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] michael-s-molina commented on a change in pull request #12010: refactor: Fix anchor-is-valid lint warnings

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #12010:
URL: https://github.com/apache/incubator-superset/pull/12010#discussion_r541238676



##########
File path: superset-frontend/src/datasource/DatasourceEditor.jsx
##########
@@ -790,12 +790,12 @@ class DatasourceEditor extends React.PureComponent {
         </Fieldset>
         {this.allowEditSource && (
           <EditLockContainer>
-            <a href="#" onClick={this.onChangeEditMode}>
+            <span role="button" tabIndex={0} onClick={this.onChangeEditMode}>

Review comment:
       Yes it does because of `role="button"` and bootstrap. The file `scaffolding.less` contains the following:
   ```
   [role="button"] {
     cursor: pointer;
   }
   ```




----------------------------------------------------------------
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] michael-s-molina commented on a change in pull request #12010: refactor: Fix anchor-is-valid lint warnings

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #12010:
URL: https://github.com/apache/incubator-superset/pull/12010#discussion_r541217048



##########
File path: superset-frontend/src/SqlLab/components/QueryTable.jsx
##########
@@ -188,22 +188,22 @@ const QueryTable = props => {
         );
         q.actions = (
           <div>
-            <Link
-              className="fa fa-pencil m-r-3"
+            <IconTooltip
+              className="fa fa-pencil m-r-3 pointer"

Review comment:
       I'll add a property because we have icons that only display information but are not clickable.




----------------------------------------------------------------
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] rusackas commented on a change in pull request #12010: refactor: Fix anchor-is-valid lint warnings

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



##########
File path: superset-frontend/src/SqlLab/components/QueryTable.jsx
##########
@@ -188,22 +188,22 @@ const QueryTable = props => {
         );
         q.actions = (
           <div>
-            <Link
-              className="fa fa-pencil m-r-3"
+            <IconTooltip
+              className="fa fa-pencil m-r-3 pointer"

Review comment:
       or... maybe the `role` attribute makes sense in this case, too?




----------------------------------------------------------------
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] rusackas commented on a change in pull request #12010: refactor: Fix anchor-is-valid lint warnings

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



##########
File path: superset-frontend/src/components/IconTooltip.tsx
##########
@@ -69,11 +74,11 @@ const Link = ({
         mouseEnterDelay={0.3}
         mouseLeaveDelay={0.15}
       >
-        {link}
+        {iconTooltip}
       </Tooltip>
     );
   }
-  return link;
+  return iconTooltip;
 };
 
-export default Link;
+export default IconTooltip;

Review comment:
       personally, I prefer named exports to better encourage uniform naming/usage on the import side... but this isn't your fault, and I'm not sure if it's worth changing 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] michael-s-molina commented on a change in pull request #12010: refactor: Fix anchor-is-valid lint warnings

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #12010:
URL: https://github.com/apache/incubator-superset/pull/12010#discussion_r542312825



##########
File path: superset-frontend/src/SqlLab/components/QueryTable.jsx
##########
@@ -188,22 +188,22 @@ const QueryTable = props => {
         );
         q.actions = (
           <div>
-            <Link
-              className="fa fa-pencil m-r-3"
+            <IconTooltip
+              className="fa fa-pencil m-r-3 pointer"

Review comment:
       I was thinking... `IconTooltip` has a `className` property and a `style` property. So I shouldn't add a `cursor` property because that is part of a style. I could pass a property value of `style={{cursor: pointer}}` but since I'm using `className` I think that passing the pointer class is better because all the style of the component is being defined in a single place.




----------------------------------------------------------------
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] rusackas commented on a change in pull request #12010: refactor: Fix anchor-is-valid lint warnings

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



##########
File path: superset-frontend/src/SqlLab/components/QueryTable.jsx
##########
@@ -188,22 +188,22 @@ const QueryTable = props => {
         );
         q.actions = (
           <div>
-            <Link
-              className="fa fa-pencil m-r-3"
+            <IconTooltip
+              className="fa fa-pencil m-r-3 pointer"

Review comment:
       if `pointer` is a bootstrap class, for the cursor, it might be good to either
   a) add a `pointer` prop to the `IconTooltip` component, and add the style conditionally there
   b) just... add the cursor style to the IconTooltip component by default if this should _always_ be the case ¯\\\_(ツ)_/¯ 




----------------------------------------------------------------
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] rusackas commented on a change in pull request #12010: refactor: Fix anchor-is-valid lint warnings

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



##########
File path: superset-frontend/spec/javascripts/sqllab/IconTooltip_spec.jsx
##########
@@ -18,22 +18,24 @@
  */
 import React from 'react';
 import { shallow } from 'enzyme';
+import { Tooltip } from 'src/common/components/Tooltip';
+import IconTooltip from 'src/components/IconTooltip';
 
-import Link from 'src/components/Link';
-
-describe('Link', () => {
+describe('IconTooltip', () => {
   const mockedProps = {
     tooltip: 'This is a tooltip',
     href: 'https://www.airbnb.com',

Review comment:
       I'm not sure if the `href` is used in the codebase anywhere... if not, then we're all good. But if it is, then we should find a good way to support it, perhaps rendering `a`/`span` conditionally.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

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] rusackas commented on a change in pull request #12010: refactor: Fix anchor-is-valid lint warnings

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



##########
File path: superset-frontend/src/SqlLab/components/QueryTable.jsx
##########
@@ -188,22 +188,22 @@ const QueryTable = props => {
         );
         q.actions = (
           <div>
-            <Link
-              className="fa fa-pencil m-r-3"
+            <IconTooltip
+              className="fa fa-pencil m-r-3 pointer"

Review comment:
       Totally agree that the `className` is better than the `style`! 100%
   
   What I was suggesting (but not at all requiring!) was adding a `pointer` prop to `IconTooltip`, i.e. `<IconTooltip pointer className=...>`
   
   The IconTooltip would pass that prop into the `StyledSpan` you added, which would then use it to render the styles needed, like so:
   
   ```
   const StyledSpan = styled.span`
     cursor: ${({ pointer }) => pointer ? 'pointer' : 'default'};
   ```
   
   ... but that may be overkill ;)




----------------------------------------------------------------
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] michael-s-molina commented on a change in pull request #12010: refactor: Fix anchor-is-valid lint warnings

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #12010:
URL: https://github.com/apache/incubator-superset/pull/12010#discussion_r541208087



##########
File path: superset-frontend/spec/javascripts/sqllab/IconTooltip_spec.jsx
##########
@@ -18,22 +18,24 @@
  */
 import React from 'react';
 import { shallow } from 'enzyme';
+import { Tooltip } from 'src/common/components/Tooltip';
+import IconTooltip from 'src/components/IconTooltip';
 
-import Link from 'src/components/Link';
-
-describe('Link', () => {
+describe('IconTooltip', () => {
   const mockedProps = {
     tooltip: 'This is a tooltip',
     href: 'https://www.airbnb.com',

Review comment:
       Actually we need to delete that line from the test because IconTooltip does not support href. If we want to use a `href` we should use a component like `react-router-dom` `Link`.




----------------------------------------------------------------
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] michael-s-molina commented on a change in pull request #12010: refactor: Fix anchor-is-valid lint warnings

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #12010:
URL: https://github.com/apache/incubator-superset/pull/12010#discussion_r541221006



##########
File path: superset-frontend/src/components/IconTooltip.tsx
##########
@@ -69,11 +74,11 @@ const Link = ({
         mouseEnterDelay={0.3}
         mouseLeaveDelay={0.15}
       >
-        {link}
+        {iconTooltip}
       </Tooltip>
     );
   }
-  return link;
+  return iconTooltip;
 };
 
-export default Link;
+export default IconTooltip;

Review comment:
       I also prefer named exports lol. I kept because of the current code style but I think we can use the same approach as typescript and use named exports for new components. Since this is a new component I'll make the change =D




----------------------------------------------------------------
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-io commented on pull request #12010: refactor: Fix anchor-is-valid lint warnings

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


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/12010?src=pr&el=h1) Report
   > Merging [#12010](https://codecov.io/gh/apache/incubator-superset/pull/12010?src=pr&el=desc) (1e57cfb) into [master](https://codecov.io/gh/apache/incubator-superset/commit/ba952ba39df47dfd32633fd4b5cf23e7ec5b40b9?el=desc) (ba952ba) will **decrease** coverage by `2.74%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/12010/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/12010?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #12010      +/-   ##
   ==========================================
   - Coverage   66.31%   63.56%   -2.75%     
   ==========================================
     Files         994      484     -510     
     Lines       49033    29744   -19289     
     Branches     4977        0    -4977     
   ==========================================
   - Hits        32515    18907   -13608     
   + Misses      16373    10837    -5536     
   + Partials      145        0     -145     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | cypress | `?` | |
   | javascript | `?` | |
   | python | `63.56% <ø> (-0.66%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/12010?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset/db\_engines/hive.py](https://codecov.io/gh/apache/incubator-superset/pull/12010/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
   | [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/incubator-superset/pull/12010/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `54.61% <0.00%> (-29.62%)` | :arrow_down: |
   | [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/incubator-superset/pull/12010/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `70.56% <0.00%> (-12.78%)` | :arrow_down: |
   | [superset/db\_engine\_specs/mysql.py](https://codecov.io/gh/apache/incubator-superset/pull/12010/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL215c3FsLnB5) | `79.59% <0.00%> (-12.25%)` | :arrow_down: |
   | [superset/db\_engine\_specs/sqlite.py](https://codecov.io/gh/apache/incubator-superset/pull/12010/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3NxbGl0ZS5weQ==) | `65.62% <0.00%> (-9.38%)` | :arrow_down: |
   | [superset/utils/celery.py](https://codecov.io/gh/apache/incubator-superset/pull/12010/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2VsZXJ5LnB5) | `96.42% <0.00%> (-3.58%)` | :arrow_down: |
   | [superset/examples/world\_bank.py](https://codecov.io/gh/apache/incubator-superset/pull/12010/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXhhbXBsZXMvd29ybGRfYmFuay5weQ==) | `97.10% <0.00%> (-2.90%)` | :arrow_down: |
   | [superset/examples/birth\_names.py](https://codecov.io/gh/apache/incubator-superset/pull/12010/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXhhbXBsZXMvYmlydGhfbmFtZXMucHk=) | `96.51% <0.00%> (-2.33%)` | :arrow_down: |
   | [superset/views/database/mixins.py](https://codecov.io/gh/apache/incubator-superset/pull/12010/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `80.70% <0.00%> (-1.76%)` | :arrow_down: |
   | [superset/result\_set.py](https://codecov.io/gh/apache/incubator-superset/pull/12010/diff?src=pr&el=tree#diff-c3VwZXJzZXQvcmVzdWx0X3NldC5weQ==) | `96.69% <0.00%> (-1.66%)` | :arrow_down: |
   | ... and [509 more](https://codecov.io/gh/apache/incubator-superset/pull/12010/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/12010?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/12010?src=pr&el=footer). Last update [ba952ba...1e57cfb](https://codecov.io/gh/apache/incubator-superset/pull/12010?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] codecov-io edited a comment on pull request #12010: refactor: Fix anchor-is-valid lint warnings

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


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/12010?src=pr&el=h1) Report
   > Merging [#12010](https://codecov.io/gh/apache/incubator-superset/pull/12010?src=pr&el=desc) (1e57cfb) into [master](https://codecov.io/gh/apache/incubator-superset/commit/ba952ba39df47dfd32633fd4b5cf23e7ec5b40b9?el=desc) (ba952ba) will **decrease** coverage by `2.75%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/12010/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/12010?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #12010      +/-   ##
   ==========================================
   - Coverage   66.31%   63.55%   -2.76%     
   ==========================================
     Files         994      484     -510     
     Lines       49033    29759   -19274     
     Branches     4977        0    -4977     
   ==========================================
   - Hits        32515    18914   -13601     
   + Misses      16373    10845    -5528     
   + Partials      145        0     -145     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | cypress | `?` | |
   | javascript | `?` | |
   | python | `63.55% <ø> (-0.66%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/12010?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset/db\_engines/hive.py](https://codecov.io/gh/apache/incubator-superset/pull/12010/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
   | [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/incubator-superset/pull/12010/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `54.61% <0.00%> (-29.62%)` | :arrow_down: |
   | [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/incubator-superset/pull/12010/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `70.56% <0.00%> (-12.78%)` | :arrow_down: |
   | [superset/db\_engine\_specs/mysql.py](https://codecov.io/gh/apache/incubator-superset/pull/12010/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL215c3FsLnB5) | `79.59% <0.00%> (-12.25%)` | :arrow_down: |
   | [superset/db\_engine\_specs/sqlite.py](https://codecov.io/gh/apache/incubator-superset/pull/12010/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3NxbGl0ZS5weQ==) | `65.62% <0.00%> (-9.38%)` | :arrow_down: |
   | [superset/utils/celery.py](https://codecov.io/gh/apache/incubator-superset/pull/12010/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2VsZXJ5LnB5) | `96.42% <0.00%> (-3.58%)` | :arrow_down: |
   | [superset/examples/world\_bank.py](https://codecov.io/gh/apache/incubator-superset/pull/12010/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXhhbXBsZXMvd29ybGRfYmFuay5weQ==) | `97.10% <0.00%> (-2.90%)` | :arrow_down: |
   | [superset/examples/birth\_names.py](https://codecov.io/gh/apache/incubator-superset/pull/12010/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXhhbXBsZXMvYmlydGhfbmFtZXMucHk=) | `96.51% <0.00%> (-2.33%)` | :arrow_down: |
   | [superset/views/database/mixins.py](https://codecov.io/gh/apache/incubator-superset/pull/12010/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `80.70% <0.00%> (-1.76%)` | :arrow_down: |
   | [superset/result\_set.py](https://codecov.io/gh/apache/incubator-superset/pull/12010/diff?src=pr&el=tree#diff-c3VwZXJzZXQvcmVzdWx0X3NldC5weQ==) | `96.69% <0.00%> (-1.66%)` | :arrow_down: |
   | ... and [502 more](https://codecov.io/gh/apache/incubator-superset/pull/12010/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/12010?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/12010?src=pr&el=footer). Last update [ba952ba...1e57cfb](https://codecov.io/gh/apache/incubator-superset/pull/12010?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] rusackas commented on a change in pull request #12010: refactor: Fix anchor-is-valid lint warnings

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



##########
File path: superset-frontend/src/SqlLab/components/TableElement.jsx
##########
@@ -209,15 +215,15 @@ class TableElement extends React.PureComponent {
           title={table.name}
           trigger={['hover']}
         >
-          <a
-            href="#"
+          <StyledSpan
+            dataTest="collapse"

Review comment:
       I think this should be a data-test attribute, for consistency. I'll add a suggestion for the test change too.
   ```suggestion
               data-test="collapse"
   ```




----------------------------------------------------------------
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] rusackas commented on a change in pull request #12010: refactor: Fix anchor-is-valid lint warnings

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



##########
File path: superset-frontend/spec/javascripts/sqllab/IconTooltip_spec.jsx
##########
@@ -18,22 +18,24 @@
  */
 import React from 'react';
 import { shallow } from 'enzyme';
+import { Tooltip } from 'src/common/components/Tooltip';
+import IconTooltip from 'src/components/IconTooltip';
 
-import Link from 'src/components/Link';
-
-describe('Link', () => {
+describe('IconTooltip', () => {
   const mockedProps = {
     tooltip: 'This is a tooltip',
     href: 'https://www.airbnb.com',

Review comment:
       It _looks like_ with the new changes to IconTooltip, this href isn't used anywhere. The tests don't assert against it, so perhaps they should?




----------------------------------------------------------------
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] michael-s-molina commented on a change in pull request #12010: refactor: Fix anchor-is-valid lint warnings

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #12010:
URL: https://github.com/apache/incubator-superset/pull/12010#discussion_r541193081



##########
File path: superset-frontend/src/datasource/DatasourceEditor.jsx
##########
@@ -790,12 +790,12 @@ class DatasourceEditor extends React.PureComponent {
         </Fieldset>
         {this.allowEditSource && (
           <EditLockContainer>
-            <a href="#" onClick={this.onChangeEditMode}>
+            <span role="button" tabIndex={0} onClick={this.onChangeEditMode}>

Review comment:
       Yes it does because of the button role.




----------------------------------------------------------------
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] rusackas commented on a change in pull request #12010: refactor: Fix anchor-is-valid lint warnings

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



##########
File path: superset-frontend/spec/javascripts/sqllab/TableElement_spec.jsx
##########
@@ -95,7 +95,7 @@ describe('TableElement', () => {
       },
     );
     expect(mockedActions.collapseTable.called).toBe(false);
-    wrapper.find('.table-name').simulate('click');
+    wrapper.find({ dataTest: 'collapse' }).simulate('click');

Review comment:
       ```suggestion
       wrapper.find({ 'data-test': 'collapse' }).simulate('click');
   ```




----------------------------------------------------------------
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] rusackas commented on a change in pull request #12010: refactor: Fix anchor-is-valid lint warnings

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



##########
File path: superset-frontend/spec/javascripts/sqllab/IconTooltip_spec.jsx
##########
@@ -18,22 +18,24 @@
  */
 import React from 'react';
 import { shallow } from 'enzyme';
+import { Tooltip } from 'src/common/components/Tooltip';
+import IconTooltip from 'src/components/IconTooltip';
 
-import Link from 'src/components/Link';
-
-describe('Link', () => {
+describe('IconTooltip', () => {
   const mockedProps = {
     tooltip: 'This is a tooltip',
     href: 'https://www.airbnb.com',

Review comment:
       I'm not sure if the `href` is used in the codebase anywhere... if not, then we're all good. But if it is, then we should find a good way to support it, perhaps rendering a/span conditionally.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

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] michael-s-molina commented on pull request #12010: refactor: Fix anchor-is-valid lint warnings

Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on pull request #12010:
URL: https://github.com/apache/incubator-superset/pull/12010#issuecomment-748943688


   @rusackas @villebro Rebased.


----------------------------------------------------------------
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] rusackas commented on a change in pull request #12010: refactor: Fix anchor-is-valid lint warnings

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



##########
File path: superset-frontend/src/datasource/DatasourceEditor.jsx
##########
@@ -790,12 +790,12 @@ class DatasourceEditor extends React.PureComponent {
         </Fieldset>
         {this.allowEditSource && (
           <EditLockContainer>
-            <a href="#" onClick={this.onChangeEditMode}>
+            <span role="button" tabIndex={0} onClick={this.onChangeEditMode}>

Review comment:
       Does this still show the 👆 cursor on hover? If not, that would be nice :)




----------------------------------------------------------------
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