refactor: icon to icons for controls #15568
Conversation
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
pkdotson
requested review from
michael-s-molina,
zhaoyongjie,
kgabryje,
suddjian and
rusackas
| @@ -281,6 +281,8 @@ export default function DateFilterLabel(props: DateFilterControlProps) { | |||
| setFrame(option.value as FrameType); | |||
| } | |||
|
|
|||
| const theme = useTheme(); | |||
|
/testenv up |
|
@suddjian Ephemeral environment spinning up at http://54.201.100.239:8080. Credentials are |
| @@ -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" /> | |||
There was a problem hiding this comment.
conditional render DatasetPhysical or DatasetVirtual from props.datasource.is_sqllab_view
There was a problem hiding this 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.
There was a problem hiding this 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).
There was a problem hiding this 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
There was a problem hiding this comment.
The original design for this control was with this specific icon.
There was a problem hiding this 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.
There was a problem hiding this comment.
I vote for consistency in design
|
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 |
|
Ephemeral environment shutdown and build artifacts deleted. |
* initial commit * Update DateFilterLabel.tsx
* initial commit * Update DateFilterLabel.tsx
* initial commit * Update DateFilterLabel.tsx
mistercrunch
added
🏷️ bot
SUMMARY
This pr migrates the old icon components to icons in the datasource panel, datelabel and collection control.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
All three of these controls can be tested in a filterbox chart. The icons that were migrated are labeled above.
ADDITIONAL INFORMATION