refactor: Bootstrap to AntD - Alert #12122

michael-s-molina · 2020-12-18T16:23:20Z

SUMMARY

Migrates Alert component from Bootstrap to AntD and creates a storybook with full controls support.

BEFORE/AFTER SCREENSHOTS

New Storybook!

Toast had a dependency on Alert. I removed this dependency by emulating the same behavior with a styled div. I also added an icon for every status.

We should consider replacing Toast code with AntD Notification.

@rusackas @junlincc @mihir174

Closes #12101

See: #10254

ADDITIONAL INFORMATION

codecov-io · 2020-12-18T16:31:42Z

Codecov Report

Merging #12122 (3e67ceb) into master (2ce7982) will increase coverage by 19.95%.
The diff coverage is 65.38%.

@@             Coverage Diff             @@
##           master   #12122       +/-   ##
===========================================
+ Coverage   53.06%   73.01%   +19.95%     
===========================================
  Files         489      553       +64     
  Lines       17314    20359     +3045     
  Branches     4482     5332      +850     
===========================================
+ Hits         9187    14866     +5679     
+ Misses       8127     5365     -2762     
- Partials        0      128      +128

Flag	Coverage Δ
cypress	`58.44% <72.64%> (+5.38%)`	⬆️
javascript	`62.42% <51.75%> (?)`

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

Impacted Files	Coverage Δ
.../src/SqlLab/components/EstimateQueryCostButton.jsx	`20.83% <ø> (ø)`
...end/src/SqlLab/components/ExploreResultsButton.jsx	`92.00% <ø> (+84.00%)`	⬆️
...et-frontend/src/SqlLab/components/QueryHistory.jsx	`50.00% <0.00%> (ø)`
...end/src/SqlLab/components/RunQueryActionButton.tsx	`64.28% <ø> (+11.50%)`	⬆️
...erset-frontend/src/SqlLab/components/SouthPane.jsx	`82.05% <ø> (+17.94%)`	⬆️
superset-frontend/src/chart/Chart.jsx	`59.64% <ø> (+1.75%)`	⬆️
superset-frontend/src/chart/ChartContainer.jsx	`100.00% <ø> (ø)`
superset-frontend/src/chart/ChartRenderer.jsx	`77.02% <0.00%> (+0.31%)`	⬆️
...perset-frontend/src/common/components/Dropdown.tsx	`54.76% <ø> (+4.76%)`	⬆️
...et-frontend/src/components/Alert/Alert.stories.tsx	`0.00% <0.00%> (ø)`
... and 501 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c9becc...3e67ceb. Read the comment docs.

superset-frontend/spec/javascripts/messageToasts/components/Toast_spec.jsx

superset-frontend/src/datasource/DatasourceEditor.jsx

superset-frontend/src/datasource/DatasourceModal.tsx

rusackas

This is looking AWESOME. Only a couple of minor questions, hoping to steer things a little more toward consistency (i.e. fewer style overrides) if it makes sense. If the ones I asked about need to be that way, then so be it, I'll approve and merge!

michael-s-molina · 2020-12-22T12:06:41Z

@rusackas Those were great observations. For me these specific properties (margins) are related to positioning of the component in a screen so they are not generic but layout based. With that in mind, Alert already have properties for this scenario: style and className. What styled is actually doing is passing a className for the Alert component but in a non direct manner so I thought of replacing that with the css property.

<Alert
     css={theme => ({
       marginTop: theme.gridUnit * 4,
       marginBottom: theme.gridUnit * 4,
     })}
     ...
/>

This makes more clear the we are positioning the element and have the benefit of autocompletion and type checking.

You were totally wright about the mismatching grid units. I changed all to 4.

Let me know if you agree with that 😉

superset-frontend/src/messageToasts/components/Toast.tsx

rusackas · 2021-01-04T23:55:10Z

@rusackas Those were great observations. For me these specific properties (margins) are related to positioning of the component in a screen so they are not generic but layout based. With that in mind, Alert already have properties for this scenario: style and className. What styled is actually doing is passing a className for the Alert component but in a non direct manner so I thought of replacing that with the css property.
<Alert
     css={theme => ({
       marginTop: theme.gridUnit * 4,
       marginBottom: theme.gridUnit * 4,
     })}
     ...
/>
This makes more clear the we are positioning the element and have the benefit of autocompletion and type checking.

You were totally wright about the mismatching grid units. I changed all to 4.

Let me know if you agree with that 😉

If these are one-off, situational tweaks, then I prefer the way you have it here, using the css prop. Somehow that looks more like a contextual tweak, whereas styled makes it look (to me, at least) more like we're creating a new variant of the component.

rusackas · 2021-01-04T23:58:03Z

I think if we add @lilykuang 's suggested tweak (nice catch!) this PR will be good to go!

michael-s-molina · 2021-01-05T13:12:38Z

@rusackas Accepted @lilykuang suggestion. I did a forced push to preserve commit history. I wish they implemented --amend in suggested changes.

ktmud · 2021-01-05T18:44:43Z

Need some design input, but have we considered using different colors by status for toasts as well? Icons are great, but still not very differentiable.

michael-s-molina · 2021-01-05T18:56:56Z

Need some design input, but have we considered using different colors by status for toasts as well? Icons are great, but still not very differentiable.

@ktmud We have considered but chose to keep the same style until we migrate Toast to AntD Notification in a specific PR.

This is how it looks like in AntD:

ktmud · 2021-01-05T19:27:05Z

Great. Good to hear!

michael-s-molina · 2021-02-05T16:36:41Z

Added new tests using React Testing Library!

michael-s-molina

Merged with master

rusackas · 2021-02-18T07:47:00Z

superset-frontend/src/components/Alert/index.tsx

+        border: 0,
+        backgroundColor: baseColor.light2,
+        '& .ant-alert-icon': {
+          marginRight: 10,


2 * gridUnit?

Here I'm following the exact design specification values. If we want to change this maybe we should talk with the design team to use multiples of 4 when dealing with margins, padding, etc.

rusackas · 2021-02-18T07:47:30Z

superset-frontend/src/components/Alert/index.tsx

+      icon={<Icon name={iconName} aria-label={`${type} icon`} />}
+      closeText={closable && <Icon name="x-small" aria-label="close icon" />}
+      css={{
+        padding: '6px 10px',


Not sure if it makes sense to round these up or down to the nearest gridUnit

Same as the previous comment.

rusackas · 2021-02-18T07:54:02Z

superset-frontend/src/SqlLab/components/ExploreResultsButton.jsx

+                'We recommend your summarize your data further before following that flow. ',
+              ) +
+              t('If activated you can use the ')}
+            <strong>CREATE TABLE AS </strong>


Suggested change

<strong>CREATE TABLE AS </strong>

<strong>{t("Create table as").toUpperCase()} </strong>

I'm guessing this would work? ¯\_(ツ)_/¯

I don't think this should be translated because it's referencing a command that doesn't have a translation.

rusackas

Noted a few minor nits, but again this PR looks great to me in general, so I'll stamp it, and circle back soon to merge whether or not those nits lead to any changes ;)

pull-request-size bot added the size/L label Dec 18, 2020

junlincc added the turing label Dec 18, 2020

rusackas requested review from rusackas, nytai and lilykuang December 18, 2020 19:43

rusackas reviewed Dec 18, 2020

View reviewed changes

superset-frontend/spec/javascripts/messageToasts/components/Toast_spec.jsx Outdated Show resolved Hide resolved

michael-s-molina force-pushed the bootstrap-to-antd-alert branch 2 times, most recently from f8cb19d to 8746a36 Compare December 21, 2020 17:26

rusackas reviewed Dec 22, 2020

View reviewed changes

superset-frontend/src/datasource/DatasourceEditor.jsx Outdated Show resolved Hide resolved

rusackas reviewed Dec 22, 2020

View reviewed changes

superset-frontend/src/datasource/DatasourceModal.tsx Outdated Show resolved Hide resolved

rusackas reviewed Dec 22, 2020

View reviewed changes

michael-s-molina force-pushed the bootstrap-to-antd-alert branch 2 times, most recently from 0f6896d to 654defb Compare December 28, 2020 12:16

michael-s-molina changed the title ~~refactor: Migrate Bootstrap Alert to AntD (#12101)~~ refactor: Bootstrap to AntD - Alert (#12101) Jan 4, 2021

lilykuang reviewed Jan 4, 2021

View reviewed changes

superset-frontend/src/messageToasts/components/Toast.tsx Outdated Show resolved Hide resolved

michael-s-molina force-pushed the bootstrap-to-antd-alert branch from 654defb to 4ff366a Compare January 5, 2021 13:10

rusackas closed this Jan 5, 2021

rusackas reopened this Jan 5, 2021

rusackas added hold! On hold need:design-review Requires input/approval from a Designer labels Jan 6, 2021

michael-s-molina force-pushed the bootstrap-to-antd-alert branch 2 times, most recently from e4a9e36 to 8c8b05b Compare January 26, 2021 21:21

junlincc added the Turing label Jan 27, 2021

rusackas removed the hold! On hold label Jan 27, 2021

michael-s-molina changed the title ~~refactor: Bootstrap to AntD - Alert (#12101)~~ refactor: Bootstrap to AntD - Alert Jan 29, 2021

michael-s-molina force-pushed the bootstrap-to-antd-alert branch 2 times, most recently from 4a0e857 to 7b708ec Compare February 2, 2021 19:36

rusackas added the design-sys-revisit label Feb 2, 2021

michael-s-molina force-pushed the bootstrap-to-antd-alert branch from 7b708ec to a8cdc91 Compare February 4, 2021 12:53

pull-request-size bot added size/XL and removed size/L labels Feb 4, 2021

michael-s-molina force-pushed the bootstrap-to-antd-alert branch 3 times, most recently from b74bb2b to 878c5a1 Compare February 5, 2021 16:35

michael-s-molina requested a review from rusackas February 5, 2021 20:06

michael-s-molina force-pushed the bootstrap-to-antd-alert branch from 878c5a1 to b53b6fb Compare February 8, 2021 19:40

michael-s-molina mentioned this pull request Feb 9, 2021

feat: add autocopy functionality to CopyToClipboard (#10328) #13037

Merged

6 tasks

rusackas added the hold! On hold label Feb 16, 2021

Migrate Bootstrap Alert to AntD (apache#12101)

3e67ceb

michael-s-molina force-pushed the bootstrap-to-antd-alert branch from b53b6fb to 3e67ceb Compare February 17, 2021 10:41

michael-s-molina commented Feb 17, 2021

View reviewed changes

rusackas reviewed Feb 18, 2021

View reviewed changes

rusackas approved these changes Feb 18, 2021

View reviewed changes

rusackas removed hold! On hold need:design-review Requires input/approval from a Designer labels Feb 18, 2021

rusackas merged commit 42ab578 into apache:master Feb 22, 2021

michael-s-molina mentioned this pull request Jan 21, 2022

feat(explore): more toast feedback on user actions in Explore #18108

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Bootstrap to AntD - Alert #12122

refactor: Bootstrap to AntD - Alert #12122

michael-s-molina commented Dec 18, 2020 •

edited

Loading

codecov-io commented Dec 18, 2020 •

edited

Loading

rusackas left a comment

michael-s-molina commented Dec 22, 2020 •

edited

Loading

rusackas commented Jan 4, 2021

rusackas commented Jan 4, 2021

michael-s-molina commented Jan 5, 2021 •

edited

Loading

ktmud commented Jan 5, 2021

michael-s-molina commented Jan 5, 2021

ktmud commented Jan 5, 2021

michael-s-molina commented Feb 5, 2021

michael-s-molina left a comment •

edited

Loading

rusackas Feb 18, 2021

michael-s-molina Feb 19, 2021

rusackas Feb 18, 2021

michael-s-molina Feb 19, 2021

rusackas Feb 18, 2021 •

edited

Loading

michael-s-molina Feb 19, 2021

rusackas left a comment

	<strong>CREATE TABLE AS </strong>
	<strong>{t("Create table as").toUpperCase()} </strong>

refactor: Bootstrap to AntD - Alert #12122

refactor: Bootstrap to AntD - Alert #12122

Conversation

michael-s-molina commented Dec 18, 2020 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS

ADDITIONAL INFORMATION

codecov-io commented Dec 18, 2020 • edited Loading

Codecov Report

rusackas left a comment

Choose a reason for hiding this comment

michael-s-molina commented Dec 22, 2020 • edited Loading

rusackas commented Jan 4, 2021

rusackas commented Jan 4, 2021

michael-s-molina commented Jan 5, 2021 • edited Loading

ktmud commented Jan 5, 2021

michael-s-molina commented Jan 5, 2021

ktmud commented Jan 5, 2021

michael-s-molina commented Feb 5, 2021

michael-s-molina left a comment • edited Loading

Choose a reason for hiding this comment

rusackas Feb 18, 2021

Choose a reason for hiding this comment

michael-s-molina Feb 19, 2021

Choose a reason for hiding this comment

rusackas Feb 18, 2021

Choose a reason for hiding this comment

michael-s-molina Feb 19, 2021

Choose a reason for hiding this comment

rusackas Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

michael-s-molina Feb 19, 2021

Choose a reason for hiding this comment

rusackas left a comment

Choose a reason for hiding this comment

michael-s-molina commented Dec 18, 2020 •

edited

Loading

codecov-io commented Dec 18, 2020 •

edited

Loading

michael-s-molina commented Dec 22, 2020 •

edited

Loading

michael-s-molina commented Jan 5, 2021 •

edited

Loading

michael-s-molina left a comment •

edited

Loading

rusackas Feb 18, 2021 •

edited

Loading