chore(explore): update Explore icons and icon colors #20612

codyml · 2022-07-05T23:13:19Z

SUMMARY

This PR changes the info and error icons on the Explore screen to AntD icons, and makes the validation error color ~~blue~~ yellow instead of red if required fields aren't filled because the user just created a new chart or selected a new viz type. If the user clears required fields that were previously filled, the validation color is still red.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

Before.mov

After:

Screen.Recording.2022-07-15.at.1.17.54.PM.mov

TESTING INSTRUCTIONS

Confirm that when creating a new chart, validation errors from missing fields are ~~blue~~ yellow instead of red.
Confirm that when changing the viz type, validation errors from missing fields are also ~~blue~~ yellow instead of red.
Confirm that new info and error icons are visible, and that no old-style info or error icons are left on the Explore screen that I missed.
Confirm that other than the above changes, the UI for Explore looks the same as before.

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- 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

codecov · 2022-07-05T23:48:23Z

Codecov Report

Merging #20612 (36edd57) into master (6e6d4e3) will increase coverage by 0.00%.
The diff coverage is 70.00%.

@@           Coverage Diff           @@
##           master   #20612   +/-   ##
=======================================
  Coverage   66.27%   66.27%           
=======================================
  Files        1757     1757           
  Lines       66955    67019   +64     
  Branches     7109     7129   +20     
=======================================
+ Hits        44374    44417   +43     
- Misses      20766    20777   +11     
- Partials     1815     1825   +10

Flag	Coverage Δ
javascript	`51.96% <70.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../src/explore/components/ControlPanelsContainer.tsx	`76.19% <65.00%> (-2.52%)`	⬇️
...-frontend/src/explore/components/ControlHeader.tsx	`81.81% <80.00%> (-1.52%)`	⬇️
...ntrols/MetricControl/AdhocMetricPopoverTrigger.tsx	`78.04% <0.00%> (-5.74%)`	⬇️
...ols/MetricControl/AdhocMetricEditPopover/index.jsx	`75.53% <0.00%> (-2.50%)`	⬇️
...ColumnSelectControl/ColumnSelectPopoverTrigger.tsx	`63.63% <0.00%> (-1.89%)`	⬇️
...ols/DndColumnSelectControl/ColumnSelectPopover.tsx	`3.26% <0.00%> (-0.49%)`	⬇️
...frontend/src/SqlLab/components/ResultSet/index.tsx	`51.38% <0.00%> (-0.36%)`	⬇️
superset-frontend/src/reports/actions/reports.js	`39.39% <0.00%> (ø)`
superset-frontend/src/components/Button/index.tsx	`100.00% <0.00%> (ø)`
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

EugeneTorap · 2022-07-06T03:50:56Z

superset-frontend/src/explore/components/ControlHeader.tsx

@@ -78,12 +82,13 @@ const ControlHeader: FC<ControlHeaderProps> = ({
      >
        {description && (
          <span>
-            <InfoTooltipWithTrigger


Remove InfoTooltipWithTrigger import in this file

It's still used on line 96 to render the lightning bolt icon, do you think I should replace that with a plain <Tooltip>...</Tooltip> as well?

EugeneTorap · 2022-07-06T03:56:46Z

Hi @codyml.
IMHO blue color uses in info icons but a required field alway uses red color to better notice it

codyml · 2022-07-06T14:13:41Z

Hi @codyml.
IMHO blue color uses in info icons but a required field alway uses red color to better notice it

Hi @EugeneTorap, The motivation for this was to not show red right away on new charts because it can make the user think they've done something wrong right from the beginning, which isn't great for user experience. But, I'm going to darken the blue color, which hopefully will make it a little more visible, and I'm also going to try to change it so it's only blue if they've never added an item to a required field on a new chart, and after that if they clear a required field on a new or existing chart it'll show the error in red. Does that sound like a good improvement?

michael-s-molina · 2022-07-08T18:15:52Z

When looking at this screen, I feel that if I click on Metric it will trigger an action just like all the other blue stuff on the screen. I understand the reasoning behind the change, I'm just not sure if the color or how we are highlighting can be improved. @kasiazjc @rusackas

codyml · 2022-07-15T19:21:16Z

When looking at this screen, I feel that if I click on Metric it will trigger an action just like all the other blue stuff on the screen. I understand the reasoning behind the change, I'm just not sure if the color or how we are highlighting can be improved. @kasiazjc @rusackas

@michael-s-molina @kasiazjc Implemented design feedback: color updated to be yellow and the label of the required field (e.g. "Metrics") stays black. "After" screen capture in PR description has been updated.

geido

Some comments below. Also found these two icons are slightly misaligned

geido · 2022-07-21T15:24:32Z

superset-frontend/src/explore/components/ControlHeader.tsx

+      hasHadNoErrors.current = true;
+    }
+
+    return hasHadNoErrors.current


We tend to avoid doing this. It would be better to unpack this logic for better readability

geido · 2022-07-21T15:25:55Z

superset-frontend/src/explore/components/ControlPanelsContainer.tsx

+import {
+  ExclamationCircleOutlined,
+  InfoCircleOutlined,
+} from '@ant-design/icons';


We don't import icons directly from Antdesign. You need to use the Icons component that comes with Antdesign icons built-in as well

Oops thanks! I've done that a couple of other times, I can open another PR that fixes the others.

If those are not too many, since we have to change them here, it might be a good idea to change them in this PR but I am happy to change the others in a separate PR as long as we fix the ones in this PR

No worries I'll do it here!

Actually, it turns out the directly-imported AntD icons render different HTML/styles from the Icons-rendered ones: there's an extra wrapper element and it resets the font size. I found 10 or so instances of directly importing AntD icons and swapping them out caused a bunch of layout bugs. I need to look into this further to try to fix the layout bug you found above, because I think that may be caused by my inconsistently switching to direct AntD import in some places in this PR, but I think switching them out across the codebase will be a more involved process that might be better done in a different PR.

… been filled in.

rusackas · 2022-07-29T04:25:11Z

/testenv up

github-actions · 2022-07-29T04:27:30Z

@rusackas Ephemeral environment spinning up at http://52.26.8.229:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

rusackas · 2022-07-29T15:03:16Z

superset-frontend/src/explore/components/ControlHeader.tsx

@@ -40,6 +40,16 @@ export type ControlHeaderProps = {
  danger?: string;
 };

+const iconStyles = css`


This is fine for now, but let's talk about this chunk in person sometime. I'd love to be in a state where we don't need to be adding/overriding/tweaking Icon styles in any particular use/implementation, instead adding classes/config/props on the Icon component itself to handle these needs more globally.

rusackas · 2022-07-29T15:04:40Z

superset-frontend/src/explore/components/ControlPanelsContainer.tsx

@@ -531,14 +588,23 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => {
              placement="right"
              title={props.errorMessage}
            >
-              <i className="fa fa-exclamation-circle text-danger fa-lg" />


I can't tell you how happy it makes me to see these fa-* icons fading away one by one :D

rusackas

LGTM!!!! THANK YOU to everyone involved, especially @codyml for all the due diligence here.

github-actions · 2022-07-29T16:03:56Z

Ephemeral environment shutdown and build artifacts deleted.

superset-github-bot bot added the preset-io label Jul 5, 2022

pull-request-size bot added the size/M label Jul 5, 2022

EugeneTorap reviewed Jul 6, 2022

View reviewed changes

codyml marked this pull request as draft July 6, 2022 14:09

codyml force-pushed the codyml/sc-51438/warnings-in-explore-data-pane-should-be-blue branch from 4223fe1 to cf01c1f Compare July 6, 2022 23:14

pull-request-size bot added size/L and removed size/M labels Jul 6, 2022

codyml marked this pull request as ready for review July 6, 2022 23:17

codyml force-pushed the codyml/sc-51438/warnings-in-explore-data-pane-should-be-blue branch from cf01c1f to 9f86ca0 Compare July 15, 2022 19:12

geido reviewed Jul 21, 2022

View reviewed changes

codyml added 5 commits July 26, 2022 14:17

Update Explore icons and icon colors.

f942447

Change shade of blue and make blue only appear when fields have never…

00ba952

… been filled in.

Fix Cypress test.

bcf2275

Update non-error validation color from blue to yellow.

1f69feb

Unpack ternary.

5e3a927

codyml force-pushed the codyml/sc-51438/warnings-in-explore-data-pane-should-be-blue branch from 9f86ca0 to 47787d2 Compare July 26, 2022 21:32

codyml requested review from michael-s-molina and ktmud as code owners July 26, 2022 21:32

codyml force-pushed the codyml/sc-51438/warnings-in-explore-data-pane-should-be-blue branch from 47787d2 to 5e3a927 Compare July 26, 2022 21:45

Replace direct AntD imports with our Icons component.

36edd57

rusackas reviewed Jul 29, 2022

View reviewed changes

rusackas approved these changes Jul 29, 2022

View reviewed changes

rusackas merged commit e7acb1a into apache:master Jul 29, 2022

mistercrunch added the 🚢 2.1.3 label Feb 18, 2024

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(explore): update Explore icons and icon colors #20612

chore(explore): update Explore icons and icon colors #20612

codyml commented Jul 5, 2022 •

edited

Loading

codecov bot commented Jul 5, 2022 •

edited

Loading

EugeneTorap Jul 6, 2022

codyml Jul 6, 2022

EugeneTorap commented Jul 6, 2022

codyml commented Jul 6, 2022 •

edited

Loading

michael-s-molina commented Jul 8, 2022

codyml commented Jul 15, 2022 •

edited

Loading

geido left a comment

geido Jul 21, 2022

geido Jul 21, 2022 •

edited

Loading

codyml Jul 21, 2022

geido Jul 22, 2022

codyml Jul 22, 2022

codyml Jul 26, 2022

rusackas commented Jul 29, 2022

github-actions bot commented Jul 29, 2022

rusackas Jul 29, 2022

rusackas Jul 29, 2022

rusackas left a comment

github-actions bot commented Jul 29, 2022

chore(explore): update Explore icons and icon colors #20612

chore(explore): update Explore icons and icon colors #20612

Conversation

codyml commented Jul 5, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Jul 5, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EugeneTorap commented Jul 6, 2022

codyml commented Jul 6, 2022 • edited Loading

michael-s-molina commented Jul 8, 2022

codyml commented Jul 15, 2022 • edited Loading

geido left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geido Jul 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rusackas commented Jul 29, 2022

github-actions bot commented Jul 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rusackas left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 29, 2022

codyml commented Jul 5, 2022 •

edited

Loading

codecov bot commented Jul 5, 2022 •

edited

Loading

codyml commented Jul 6, 2022 •

edited

Loading

codyml commented Jul 15, 2022 •

edited

Loading

geido Jul 21, 2022 •

edited

Loading