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/31 00:16:50 UTC

[GitHub] [incubator-superset] rusackas opened a new pull request #10486: AntD-based Card component (early version)

rusackas opened a new pull request #10486:
URL: https://github.com/apache/incubator-superset/pull/10486


   ### SUMMARY
   <!--- Describe the change below, including rationale and design decisions -->
   
   This PR does a few things:
   1) Adds to the webpack config, allowing unit tests to be run from alongside component files.
   2) Adds a couple tweaks to React Storybook
   3) The point of all this: a proto-component for Cards, using AntD. This will be expanded on to _actually_ use in the UI in subsequent PRs for CRUD list/card views.
   
   ![image](https://user-images.githubusercontent.com/812905/88986614-5912a200-d288-11ea-9e78-dfb1375538fa.png)
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   <!--- Skip this if not applicable -->
   
   ### TEST PLAN
   <!--- What steps should be taken to verify the changes -->
   Tested/viewable in Storybook! `npm run storybook`
   Basic unit tests build from storybook stories.
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] 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] mistercrunch commented on pull request #10486: feat: AntD-based Card component (early/proto version)

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


   I'd like to see the prod bundle size (`npm run build`) before / after the antd import to make sure treeshaking is working right


----------------------------------------------------------------
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] nruhe edited a comment on pull request #10486: feat: AntD-based Card component (early/proto version)

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


   > reducing the props to a subset of the ones exposed by antd helps simplifying the component interface surface, creates more visual consistency in Superset
   
   @mistercrunch Aside from the style props, we should generally avoid limiting the properties antd exposes to us. Doing so just makes the components less flexible and will lead to bad designs in the long run.


----------------------------------------------------------------
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 edited a comment on pull request #10486: feat: AntD-based Card component (early/proto version)

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


   Overall the approach of wrapping `antd` components that we want to use is going to be a fair amount of boilerplate code, but I think it's worth it overall for the flexibility it offers:
   - reducing the props to a subset of the ones exposed by antd helps simplifying the component interface surface, creates more visual consistency in Superset
   - allows for rewiring/styling abstraction to be all in one place
   - ease the transition from bootrap/react-boostrap to AntD
   - {more! just can't think of it just 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] nruhe edited a comment on pull request #10486: feat: AntD-based Card component (early/proto version)

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


   > reducing the props to a subset of the ones exposed by antd helps simplifying the component interface surface, creates more visual consistency in Superset
   
   @mistercrunch Aside from the style props, we should generally avoid limiting the properties antd exposes to us. Doing so just makes the components less flexible and will lead to bad designs in the long run. Looking at you `QueryButton, Button, BootstrapButton, SubmitButton`


----------------------------------------------------------------
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 #10486: feat: AntD-based Card component (early/proto version)

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


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10486?src=pr&el=h1) Report
   > Merging [#10486](https://codecov.io/gh/apache/incubator-superset/pull/10486?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/6f56cd5e9d18ab8c89f282551b349ccf8da99dc1&el=desc) will **increase** coverage by `0.00%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/10486/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/10486?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #10486      +/-   ##
   ==========================================
     Coverage   70.25%   70.25%              
   ==========================================
     Files         605      196     -409     
     Lines       32377    19063   -13314     
     Branches     3271        0    -3271     
   ==========================================
   - Hits        22745    13392    -9353     
   + Misses       9522     5671    -3851     
   + Partials      110        0     -110     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #cypress | `?` | |
   | #javascript | `?` | |
   | #python | `70.25% <ø> (+0.42%)` | :arrow_up: |
   
   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/10486?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset/views/alerts.py](https://codecov.io/gh/apache/incubator-superset/pull/10486/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvYWxlcnRzLnB5) | `70.83% <0.00%> (-29.17%)` | :arrow_down: |
   | [superset/views/database/mixins.py](https://codecov.io/gh/apache/incubator-superset/pull/10486/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `59.64% <0.00%> (-21.06%)` | :arrow_down: |
   | [superset/db\_engine\_specs/mysql.py](https://codecov.io/gh/apache/incubator-superset/pull/10486/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL215c3FsLnB5) | `78.72% <0.00%> (-12.77%)` | :arrow_down: |
   | [superset/views/database/validators.py](https://codecov.io/gh/apache/incubator-superset/pull/10486/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvdmFsaWRhdG9ycy5weQ==) | `78.94% <0.00%> (-5.27%)` | :arrow_down: |
   | [superset/db\_engine\_specs/postgres.py](https://codecov.io/gh/apache/incubator-superset/pull/10486/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3Bvc3RncmVzLnB5) | `97.36% <0.00%> (-2.64%)` | :arrow_down: |
   | [superset/databases/api.py](https://codecov.io/gh/apache/incubator-superset/pull/10486/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGF0YWJhc2VzL2FwaS5weQ==) | `85.18% <0.00%> (-2.00%)` | :arrow_down: |
   | [superset/models/core.py](https://codecov.io/gh/apache/incubator-superset/pull/10486/diff?src=pr&el=tree#diff-c3VwZXJzZXQvbW9kZWxzL2NvcmUucHk=) | `85.27% <0.00%> (-1.95%)` | :arrow_down: |
   | [superset/charts/dao.py](https://codecov.io/gh/apache/incubator-superset/pull/10486/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY2hhcnRzL2Rhby5weQ==) | `87.17% <0.00%> (-1.20%)` | :arrow_down: |
   | [superset/jinja\_context.py](https://codecov.io/gh/apache/incubator-superset/pull/10486/diff?src=pr&el=tree#diff-c3VwZXJzZXQvamluamFfY29udGV4dC5weQ==) | `80.00% <0.00%> (-0.96%)` | :arrow_down: |
   | [superset/charts/api.py](https://codecov.io/gh/apache/incubator-superset/pull/10486/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY2hhcnRzL2FwaS5weQ==) | `73.89% <0.00%> (-0.69%)` | :arrow_down: |
   | ... and [445 more](https://codecov.io/gh/apache/incubator-superset/pull/10486/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10486?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/10486?src=pr&el=footer). Last update [6f56cd5...a96e8b4](https://codecov.io/gh/apache/incubator-superset/pull/10486?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] mistercrunch commented on a change in pull request #10486: feat: AntD-based Card component (early/proto version)

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



##########
File path: superset-frontend/.storybook/preview.jsx
##########
@@ -29,4 +31,14 @@ const themeDecorator = storyFn => (
 );
 
 addDecorator(jsxDecorator);
-addDecorator(themeDecorator);
\ No newline at end of file
+addDecorator(themeDecorator);
+addDecorator(withPaddings);
+
+addParameters({
+  paddings: [
+    { name: 'None', value: '0px' },
+    { name: 'Small', value: '16px' },
+    { name: 'Medium', value: '32px', default: true },
+    { name: 'Large', value: '64px' },
+  ],
+});

Review comment:
       I think the python pre-commit hooks get these EoF lints for me




----------------------------------------------------------------
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 edited a comment on pull request #10486: feat: AntD-based Card component (early/proto version)

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


   I'd like to see the prod bundle sizes (`npm run build`) before / after the antd import to make sure treeshaking is working right


----------------------------------------------------------------
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 pull request #10486: feat: AntD-based Card component (early/proto version)

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


   Overall the approach of wrapping all `antd` components is going to be a fair amount of boilerplate code, but I think it's worth it overall for the flexibility it offers:
   - reducing the props to a subset of the ones exposed by antd helps simplifying the component interface surface, creates more visual consistency in Superset
   - allows for rewiring/styling abstraction to be all in one place
   - {more! just can't think of it just 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] nruhe commented on pull request #10486: feat: AntD-based Card component (early/proto version)

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


   > reducing the props to a subset of the ones exposed by antd helps simplifying the component interface surface, creates more visual consistency in Superset
   
   @mistercrunch Aside from the style props, we should generally avoid limiting the properties antd exposes to us. Doing so just makes the components more rigid and will lead to bad designs in the long run.


----------------------------------------------------------------
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 edited a comment on pull request #10486: feat: AntD-based Card component (early/proto version)

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


   Overall the approach of wrapping `antd` components we use is going to be a fair amount of boilerplate code, but I think it's worth it overall for the flexibility it offers:
   - reducing the props to a subset of the ones exposed by antd helps simplifying the component interface surface, creates more visual consistency in Superset
   - allows for rewiring/styling abstraction to be all in one place
   - {more! just can't think of it just 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] rusackas closed pull request #10486: feat: AntD-based Card component (early/proto version)

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


   


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