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 2021/07/07 08:24:55 UTC
[GitHub] [superset] pkdotson opened a new pull request #15568: refactor: icon to icons for controls
pkdotson opened a new pull request #15568:
URL: https://github.com/apache/superset/pull/15568
### SUMMARY
This pr migrates the old icon components to icons in the datasource panel, datelabel and collection control.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<img width="505" alt="Screen Shot 2021-07-07 at 1 23 19 AM" src="https://user-images.githubusercontent.com/17326228/124725722-f37c5c00-dec1-11eb-8c4c-a2cb9abfc1fe.png">
### TESTING INSTRUCTIONS
All three of these controls can be tested in a filterbox chart. The icons that were migrated are labeled above.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] 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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] michael-s-molina commented on a change in pull request #15568: refactor: icon to icons for controls
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #15568:
URL: https://github.com/apache/superset/pull/15568#discussion_r666490504
##########
File path: superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx
##########
@@ -190,7 +195,7 @@ class DatasourceControl extends React.PureComponent {
return (
<Styles data-test="datasource-control" className="DatasourceControl">
<div className="data-container">
- <Icon name="dataset-physical" className="dataset-svg" />
+ <Icons.DatasetPhysical className="dataset-svg" />
Review comment:
One question that we can ask is: In the context of Explore, is important for the user to know if he's working on a physical or virtual dataset? Is there any difference in terms of functionality? @rusackas @betodealmeida
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] github-actions[bot] commented on pull request #15568: refactor: icon to icons for controls
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15568:
URL: https://github.com/apache/superset/pull/15568#issuecomment-876808794
Ephemeral environment shutdown and build artifacts deleted.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] suddjian commented on a change in pull request #15568: refactor: icon to icons for controls
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #15568:
URL: https://github.com/apache/superset/pull/15568#discussion_r665739936
##########
File path: superset-frontend/src/explore/components/controls/DateFilterControl/DateFilterLabel.tsx
##########
@@ -281,6 +281,8 @@ export default function DateFilterLabel(props: DateFilterControlProps) {
setFrame(option.value as FrameType);
}
+ const theme = useTheme();
Review comment:
nice catch 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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] michael-s-molina commented on a change in pull request #15568: refactor: icon to icons for controls
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #15568:
URL: https://github.com/apache/superset/pull/15568#discussion_r666490504
##########
File path: superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx
##########
@@ -190,7 +195,7 @@ class DatasourceControl extends React.PureComponent {
return (
<Styles data-test="datasource-control" className="DatasourceControl">
<div className="data-container">
- <Icon name="dataset-physical" className="dataset-svg" />
+ <Icons.DatasetPhysical className="dataset-svg" />
Review comment:
One question that we can ask is: In the context of Explore, is important for the user to know if he's working on a physical or virtual dataset? Is there any difference in terms of functionality?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] codecov[bot] commented on pull request #15568: refactor: icon to icons for controls
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #15568:
URL: https://github.com/apache/superset/pull/15568#issuecomment-875804638
# [Codecov](https://codecov.io/gh/apache/superset/pull/15568?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#15568](https://codecov.io/gh/apache/superset/pull/15568?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2b5886b) into [master](https://codecov.io/gh/apache/superset/commit/83be06d2ccdd9b8d6200d95e383f5cd847166e3a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (83be06d) will **decrease** coverage by `0.22%`.
> The diff coverage is `100.00%`.
> :exclamation: Current head 2b5886b differs from pull request most recent head 6f276cf. Consider uploading reports for the commit 6f276cf to get more accurate results
[](https://codecov.io/gh/apache/superset/pull/15568?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #15568 +/- ##
==========================================
- Coverage 76.95% 76.72% -0.23%
==========================================
Files 976 976
Lines 51290 51296 +6
Branches 6907 6907
==========================================
- Hits 39468 39356 -112
- Misses 11603 11721 +118
Partials 219 219
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| javascript | `71.42% <100.00%> (+<0.01%)` | :arrow_up: |
| presto | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/15568?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...re/components/controls/CollectionControl/index.jsx](https://codecov.io/gh/apache/superset/pull/15568/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9Db2xsZWN0aW9uQ29udHJvbC9pbmRleC5qc3g=) | `83.33% <ø> (ø)` | |
| [superset-frontend/src/components/Tabs/Tabs.tsx](https://codecov.io/gh/apache/superset/pull/15568/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvVGFicy9UYWJzLnRzeA==) | `96.77% <100.00%> (+0.22%)` | :arrow_up: |
| [.../components/Header/HeaderActionsDropdown/index.jsx](https://codecov.io/gh/apache/superset/pull/15568/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL0hlYWRlci9IZWFkZXJBY3Rpb25zRHJvcGRvd24vaW5kZXguanN4) | `67.50% <100.00%> (+0.41%)` | :arrow_up: |
| [...re/components/controls/DatasourceControl/index.jsx](https://codecov.io/gh/apache/superset/pull/15568/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9EYXRhc291cmNlQ29udHJvbC9pbmRleC5qc3g=) | `87.17% <100.00%> (+0.33%)` | :arrow_up: |
| [...nts/controls/DateFilterControl/DateFilterLabel.tsx](https://codecov.io/gh/apache/superset/pull/15568/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9jb250cm9scy9EYXRlRmlsdGVyQ29udHJvbC9EYXRlRmlsdGVyTGFiZWwudHN4) | `66.93% <100.00%> (+0.26%)` | :arrow_up: |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/superset/pull/15568/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-82.15%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/superset/pull/15568/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `69.44% <0.00%> (-17.07%)` | :arrow_down: |
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/superset/pull/15568/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `83.36% <0.00%> (-6.95%)` | :arrow_down: |
| [superset/views/database/mixins.py](https://codecov.io/gh/apache/superset/pull/15568/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdmlld3MvZGF0YWJhc2UvbWl4aW5zLnB5) | `81.03% <0.00%> (-1.73%)` | :arrow_down: |
| [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/superset/pull/15568/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `88.26% <0.00%> (-1.65%)` | :arrow_down: |
| ... and [3 more](https://codecov.io/gh/apache/superset/pull/15568/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/15568?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/15568?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [83be06d...6f276cf](https://codecov.io/gh/apache/superset/pull/15568?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] betodealmeida commented on a change in pull request #15568: refactor: icon to icons for controls
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #15568:
URL: https://github.com/apache/superset/pull/15568#discussion_r666671529
##########
File path: superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx
##########
@@ -190,7 +195,7 @@ class DatasourceControl extends React.PureComponent {
return (
<Styles data-test="datasource-control" className="DatasourceControl">
<div className="data-container">
- <Icon name="dataset-physical" className="dataset-svg" />
+ <Icons.DatasetPhysical className="dataset-svg" />
Review comment:
@michael-s-molina it shouldn't matter, everything the user does on a physical dataset in Explore they should be able to in a virtual, and vice-versa.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] michael-s-molina commented on a change in pull request #15568: refactor: icon to icons for controls
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #15568:
URL: https://github.com/apache/superset/pull/15568#discussion_r666484002
##########
File path: superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx
##########
@@ -190,7 +195,7 @@ class DatasourceControl extends React.PureComponent {
return (
<Styles data-test="datasource-control" className="DatasourceControl">
<div className="data-container">
- <Icon name="dataset-physical" className="dataset-svg" />
+ <Icons.DatasetPhysical className="dataset-svg" />
Review comment:
I'm fine either way, but I liked @zhaoyongjie suggestion. The icons are similar enough to let the user know that they represent a dataset. With different icons, we have additional information (virtual or physical).
<img width="38" alt="Screen Shot 2021-07-08 at 5 04 54 PM" src="https://user-images.githubusercontent.com/70410625/124983675-a3f77680-e00e-11eb-9a74-a9ea67f85523.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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] michael-s-molina commented on a change in pull request #15568: refactor: icon to icons for controls
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #15568:
URL: https://github.com/apache/superset/pull/15568#discussion_r666484002
##########
File path: superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx
##########
@@ -190,7 +195,7 @@ class DatasourceControl extends React.PureComponent {
return (
<Styles data-test="datasource-control" className="DatasourceControl">
<div className="data-container">
- <Icon name="dataset-physical" className="dataset-svg" />
+ <Icons.DatasetPhysical className="dataset-svg" />
Review comment:
I'm fine either way, but I liked @zhaoyongjie suggestion. The icons are similar in a way that the user knows that they represent a dataset. With different icons, we have additional information (virtual or physical).
<img width="38" alt="Screen Shot 2021-07-08 at 5 04 54 PM" src="https://user-images.githubusercontent.com/70410625/124983675-a3f77680-e00e-11eb-9a74-a9ea67f85523.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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] suddjian commented on pull request #15568: refactor: icon to icons for controls
Posted by GitBox <gi...@apache.org>.
suddjian commented on pull request #15568:
URL: https://github.com/apache/superset/pull/15568#issuecomment-875970303
/testenv up
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] github-actions[bot] commented on pull request #15568: refactor: icon to icons for controls
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15568:
URL: https://github.com/apache/superset/pull/15568#issuecomment-875971213
@suddjian Ephemeral environment spinning up at http://54.201.100.239:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] zhaoyongjie commented on a change in pull request #15568: refactor: icon to icons for controls
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on a change in pull request #15568:
URL: https://github.com/apache/superset/pull/15568#discussion_r665856636
##########
File path: superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx
##########
@@ -190,7 +195,7 @@ class DatasourceControl extends React.PureComponent {
return (
<Styles data-test="datasource-control" className="DatasourceControl">
<div className="data-container">
- <Icon name="dataset-physical" className="dataset-svg" />
+ <Icons.DatasetPhysical className="dataset-svg" />
Review comment:
conditional render `DatasetPhysical` or `DatasetVirtual` from `props.datasource.is_sqllab_view`
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] rusackas commented on pull request #15568: refactor: icon to icons for controls
Posted by GitBox <gi...@apache.org>.
rusackas commented on pull request #15568:
URL: https://github.com/apache/superset/pull/15568#issuecomment-876808656
I think the question of whether or not we should render the relevant icon for the particular dataset is an interesting one. I can see both sides of the argument. If I were forced to choose a side, I'd say I would _personally_ lean toward making it accurately reflect the dataset.
_However_ this is not the intent of this PR! And it's clearly a bit contentious. We're waging a war to migrate to a new component, and this PR hits the mark. I agree the change we're discussing is worth the discussion... but maybe we should do that on an Issue/slack/zoom/etc, and open a separate PR if we want to pursue it after further discussion.
Merging this one for now... baby steps :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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] michael-s-molina commented on a change in pull request #15568: refactor: icon to icons for controls
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on a change in pull request #15568:
URL: https://github.com/apache/superset/pull/15568#discussion_r666484002
##########
File path: superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx
##########
@@ -190,7 +195,7 @@ class DatasourceControl extends React.PureComponent {
return (
<Styles data-test="datasource-control" className="DatasourceControl">
<div className="data-container">
- <Icon name="dataset-physical" className="dataset-svg" />
+ <Icons.DatasetPhysical className="dataset-svg" />
Review comment:
I'm fine either way, but I liked @zhaoyongjie suggestion. The icons are similar in a way that the user knows that they represent a dataset but with different icons, we have additional information (virtual or physical).
<img width="38" alt="Screen Shot 2021-07-08 at 5 04 54 PM" src="https://user-images.githubusercontent.com/70410625/124983675-a3f77680-e00e-11eb-9a74-a9ea67f85523.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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] suddjian commented on a change in pull request #15568: refactor: icon to icons for controls
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #15568:
URL: https://github.com/apache/superset/pull/15568#discussion_r666478260
##########
File path: superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx
##########
@@ -190,7 +195,7 @@ class DatasourceControl extends React.PureComponent {
return (
<Styles data-test="datasource-control" className="DatasourceControl">
<div className="data-container">
- <Icon name="dataset-physical" className="dataset-svg" />
+ <Icons.DatasetPhysical className="dataset-svg" />
Review comment:
I disagree, I think having a consistent icon here adds clarity. To me the icon is doing the job of communicating "this is the dataset control" rather than "this is the type of this dataset". But that decision should probably be left to design folks.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] pkdotson commented on a change in pull request #15568: refactor: icon to icons for controls
Posted by GitBox <gi...@apache.org>.
pkdotson commented on a change in pull request #15568:
URL: https://github.com/apache/superset/pull/15568#discussion_r666533396
##########
File path: superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx
##########
@@ -190,7 +195,7 @@ class DatasourceControl extends React.PureComponent {
return (
<Styles data-test="datasource-control" className="DatasourceControl">
<div className="data-container">
- <Icon name="dataset-physical" className="dataset-svg" />
+ <Icons.DatasetPhysical className="dataset-svg" />
Review comment:
The original design for this control was with this specific 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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] rusackas merged pull request #15568: refactor: icon to icons for controls
Posted by GitBox <gi...@apache.org>.
rusackas merged pull request #15568:
URL: https://github.com/apache/superset/pull/15568
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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] [superset] zhaoyongjie commented on a change in pull request #15568: refactor: icon to icons for controls
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on a change in pull request #15568:
URL: https://github.com/apache/superset/pull/15568#discussion_r666675175
##########
File path: superset-frontend/src/explore/components/controls/DatasourceControl/index.jsx
##########
@@ -190,7 +195,7 @@ class DatasourceControl extends React.PureComponent {
return (
<Styles data-test="datasource-control" className="DatasourceControl">
<div className="data-container">
- <Icon name="dataset-physical" className="dataset-svg" />
+ <Icons.DatasetPhysical className="dataset-svg" />
Review comment:
I vote for consistency in design
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
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