style: new toast design closer to SIP-34 #10178

lilykuang · 2020-06-26T21:55:53Z

SUMMARY

This PR is still work in process but I'd like to have early feedback.

update Toast style to match SIP-34
change Toast and ToastPresenter to typescript
move toast.less to emotion style
add Icon and style for error/warning and success toast

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

After
Success Toast

Error/Warning Toast

TEST PLAN

toggle ENABLE_REACT_CRUD_VIEWS config flag, see changes in dataset/chart view

ADDITIONAL INFORMATION

nytai · 2020-06-26T22:20:58Z

superset-frontend/src/components/ListView/Filters.tsx

@@ -145,8 +145,11 @@ function SelectFilter({
          data-test="filters-select"
          themeConfig={filterSelectTheme}
          stylesConfig={filterSelectStyles}
+          // @ts-ignore


It seems I introduced these issues when I added https://github.com/vtaits/react-select-async-paginate in #10035 and this PR merely uncovered them (for some reason... 🤔). I'll address in another PR if it's possible, the types for that package are quite confusing.

codecov-commenter · 2020-06-26T22:22:14Z

Codecov Report

Merging #10178 into master will decrease coverage by 0.46%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #10178      +/-   ##
==========================================
- Coverage   70.15%   69.68%   -0.47%     
==========================================
  Files         598      194     -404     
  Lines       32004    18862   -13142     
  Branches     3236        0    -3236     
==========================================
- Hits        22451    13144    -9307     
+ Misses       9448     5718    -3730     
+ Partials      105        0     -105

Flag	Coverage Δ
#cypress	`?`
#javascript	`?`
#python	`69.68% <ø> (+0.06%)`	⬆️

Impacted Files	Coverage Δ
.../src/dashboard/util/getFilterScopeFromNodesTree.js
...ontend/src/explore/components/QueryAndSaveBtns.jsx
...hboard/components/menu/BackgroundStyleDropdown.jsx
...nd/src/dashboard/components/ColorComponentPane.jsx
superset-frontend/src/SqlLab/constants.js
superset-frontend/src/components/Loading.tsx
...dashboard/components/FilterIndicatorsContainer.jsx
...et-frontend/src/dashboard/components/Dashboard.jsx
...shboard/components/filterscope/FilterFieldItem.jsx
...src/dashboard/components/HeaderActionsDropdown.jsx
... and 394 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 8d9bb5f...62da4ba. Read the comment docs.

etr2460

i'm a little concerned with how large the package-lock diff is here, i wouldn't expect the dep changes to cause such a large change. Can you validate to make sure this is correct?

etr2460 · 2020-06-30T02:18:20Z

superset-frontend/package.json

@@ -61,14 +61,14 @@
    "@emotion/core": "^10.0.28",
    "@superset-ui/chart": "^0.14.1",
    "@superset-ui/chart-composition": "^0.14.1",
-    "@superset-ui/color": "^0.14.2",
+    "@superset-ui/chart-controls": "^0.14.1",
+    "@superset-ui/color": "^0.14.1",


were the downgrades here intentional?

@etr2460 I didn't clean up conflict correctly. All package should be up to date now.

etr2460 · 2020-06-30T02:19:16Z

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

+
+interface ToastPresenterProps {
+  toast: { id: string; toastType: ToastType; text: string; duration: number };
+  onCloseToast: (arg0: string) => void;


let's name this argument

etr2460 · 2020-06-30T02:20:57Z

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

+
+const ToastPresenter = ({ toasts, removeToast }: ToastPresenterProps) =>
+  toasts.length > 0 && (
+    <StyledToastPresenter id="toast-presenter">


Do you think using a Portal for this might make it easier?

Although looking at the diff, it seems like this was written like this before, so up to you if you want to see if a Portal is easier or not

I think I will keep this way

etr2460

thanks for fixing the package lock conflict, a couple more comments

etr2460 · 2020-07-07T05:07:38Z

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

@@ -97,13 +117,17 @@ class Toast extends React.Component {
          toastType === DANGER_TOAST && 'toast--danger',


Are these classes still needed? It looks like they're unused now

etr2460 · 2020-07-07T05:08:30Z

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

+          {toastType === SUCCESS_TOAST ? (
+            <Icon name="check" />
+          ) : (
+            <Icon name="error" />
+          )}


Since we have a warning icon, should we use that for the WARNING_TOASTs too? And perhaps add an info icon as well? If we don't add the info icon, should we perhaps default to no icon at all, since it could be confusing for an info toast to show an error icon

I will prefer info toasts without Icon and warning toasts will have the same icon as error.

etr2460 · 2020-07-07T05:09:39Z

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

+  }
+`;
+
+type ToastType =


this is defined twice, let's pull it into a types.ts file

etr2460 · 2020-07-07T05:12:55Z

one more design nit, not sure if it's worth fixing though... It seems like the x isn't vertically centered in the toast. It's easier to see when enlarged:

etr2460 · 2020-07-07T16:59:32Z

superset-frontend/images/icons/check.svg

+  under the License.
+-->
+<svg width="24" height="24" viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg">
+<path fill-rule="evenodd" clip-rule="evenodd" d="M12 2C6.47715 2 2 6.47715 2 12C2 17.5228 6.47715 22 12 22C17.5228 22 22 17.5228 22 12C22 9.34784 20.9464 6.8043 19.0711 4.92893C17.1957 3.05357 14.6522 2 12 2Z" fill="#666666"/>


One more comment, I think you might need to default the fill attribute to "currentColor" so that we can override the color of the icon with CSS

CSS rules end up overriding the property. However, it's best to be consistent and use currentColor.

ah, I see. I was using color to change the icon's color and not fill so it didn't work. Do we prefer to override this with color or fill on the icon? i was planning a change to the Icon component to get rid of styled and use this method instead; with this then color works:

const Icon = ({ name, color = '#666666', ...rest }: IconProps) => { const Component = iconsRegistry[name]; return <Component color={color} {...rest} />; }; export default Icon;

color seems reasonable, as that's how the icon fonts behave. We should def try to use currentColor for fill rules when it makes sense as it allows for the most flexibility.

nytai

lgtm

etr2460

lgtm too. thanks for the iterations!

superset-github-bot bot added the preset-io label Jun 26, 2020

lilykuang marked this pull request as draft June 26, 2020 21:59

nytai reviewed Jun 26, 2020

View reviewed changes

lilykuang added 2 commits June 29, 2020 09:32

update toast design to dataset redesign

7d8aaa2

update superset-ui for z-index and opacity

ceb0b76

lilykuang force-pushed the lily/dataset-toast branch from 8b67b9b to ceb0b76 Compare June 29, 2020 16:42

update tabbed sql editor spec

187776a

lilykuang force-pushed the lily/dataset-toast branch from 37e653f to 187776a Compare June 29, 2020 19:00

lilykuang marked this pull request as ready for review June 29, 2020 19:17

lilykuang changed the title ~~style: new toast design closer to SIP-34~~ style: [WIP] new toast design closer to SIP-34 Jun 29, 2020

willbarrett requested review from rusackas and ktmud June 29, 2020 19:25

etr2460 reviewed Jun 30, 2020

View reviewed changes

update superset-ui packages

9c98569

pull-request-size bot added the size/L label Jun 30, 2020

lilykuang added 2 commits June 29, 2020 22:51

update spec

e156209

add error toast with icon and style

7e0df38

lilykuang requested review from etr2460 and nytai July 7, 2020 01:09

lilykuang changed the title ~~style: [WIP] new toast design closer to SIP-34~~ style: new toast design closer to SIP-34 Jul 7, 2020

etr2460 reviewed Jul 7, 2020

View reviewed changes

move ToastType to types

e8d3354

lilykuang force-pushed the lily/dataset-toast branch from 62da4ba to 2aba392 Compare July 9, 2020 04:02

pull-request-size bot added size/XL and removed size/L labels Jul 9, 2020

fix merge conflict

8b6a7d6

lilykuang force-pushed the lily/dataset-toast branch from 2aba392 to 8b6a7d6 Compare July 9, 2020 05:23

lilykuang and others added 4 commits July 9, 2020 13:45

update toast spec

1bf1424

fix spec

ed45184

add back async testing code

dcd0046

update error icon fill to current color

156a814

lilykuang requested a review from etr2460 July 10, 2020 21:25

update icons fill to current color

18667df

lilykuang closed this Jul 10, 2020

lilykuang reopened this Jul 10, 2020

nytai approved these changes Jul 10, 2020

View reviewed changes

etr2460 approved these changes Jul 10, 2020

View reviewed changes

nytai merged commit 3b4df51 into apache:master Jul 10, 2020

nytai deleted the lily/dataset-toast branch July 10, 2020 21:59

auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020

style: new toast design closer to SIP-34 (apache#10178)

9d02241

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

style: new toast design closer to SIP-34 #10178

style: new toast design closer to SIP-34 #10178

lilykuang commented Jun 26, 2020 •

edited

nytai Jun 26, 2020

codecov-commenter commented Jun 26, 2020 •

edited

etr2460 left a comment

etr2460 Jun 30, 2020

lilykuang Jun 30, 2020

etr2460 Jun 30, 2020

etr2460 Jun 30, 2020

etr2460 Jun 30, 2020

lilykuang Jul 6, 2020

etr2460 left a comment

etr2460 Jul 7, 2020

etr2460 Jul 7, 2020

lilykuang Jul 9, 2020

etr2460 Jul 7, 2020

etr2460 commented Jul 7, 2020

etr2460 Jul 7, 2020

nytai Jul 9, 2020

etr2460 Jul 9, 2020 •

edited

nytai Jul 9, 2020

nytai left a comment

etr2460 left a comment

		@@ -97,13 +117,17 @@ class Toast extends React.Component {
		toastType === DANGER_TOAST && 'toast--danger',

style: new toast design closer to SIP-34 #10178

style: new toast design closer to SIP-34 #10178

Conversation

lilykuang commented Jun 26, 2020 • edited

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

Choose a reason for hiding this comment

codecov-commenter commented Jun 26, 2020 • edited

Codecov Report

etr2460 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etr2460 left a 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

Choose a reason for hiding this comment

etr2460 commented Jul 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etr2460 Jul 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nytai left a comment

Choose a reason for hiding this comment

etr2460 left a comment

Choose a reason for hiding this comment

lilykuang commented Jun 26, 2020 •

edited

codecov-commenter commented Jun 26, 2020 •

edited

etr2460 Jul 9, 2020 •

edited