Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🪟 🎨 Update toast design #19980

Merged
merged 30 commits into from
Dec 8, 2022
Merged

🪟 🎨 Update toast design #19980

merged 30 commits into from
Dec 8, 2022

Conversation

dizel852
Copy link
Contributor

@dizel852 dizel852 commented Dec 1, 2022

What

Resolves #18631
Closes #19277

Design:
https://www.figma.com/file/gMNRYVJav25pdsoxAyW7Qp/00_01_WEB-APP-LIBRARIES?node-id=1475%3A2667&t=SvkHvP2U7YBoxfkU-0

How

Adds new Toast design + update Notification Service interface + update storybook

Types were added:

image

Notification examples:

Screenshot at Dec 02 00-40-09

Screenshot at Dec 02 00-33-22

@octavia-squidington-iv octavia-squidington-iv added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Dec 1, 2022
@dizel852
Copy link
Contributor Author

dizel852 commented Dec 1, 2022

❓ Just to clarify: chatted with @Upmitt , and we have a design for "errored warning" and "grey info" toasts.
Do we need to add these kinds of toasts also?
image

❓ One more clarifications is needed:
@edmundito in #18631 you've mentioned that:

Reduce the bottom margin of the page ($spacing-page-bottom in scss) to be enough for the toast + Intercom badge (>= 88px) - Design

Do we need to reduce or increase? Atm we have 49px margin and $spacing-page-bottom: 150px;? So, what should I do? 😅
image

@dizel852 dizel852 marked this pull request as ready for review December 1, 2022 23:09
@dizel852 dizel852 requested a review from a team as a code owner December 1, 2022 23:09
Copy link
Contributor

@edmundito edmundito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a review and posted a few notes.

I will follow up with you about the positioning and the margin issue.

I tried stress testing the toast content and looks like it may nee more work for cases when there's a lot of text:
image

  • Should the text be centered?
  • Should the icon be vertically aligned?
  • Should the icon and the button protect their width?

airbyte-webapp/src/components/ui/Toast/Toast.module.scss Outdated Show resolved Hide resolved
airbyte-webapp/src/hooks/services/Notification/types.ts Outdated Show resolved Hide resolved
airbyte-webapp/src/components/ui/Toast/Toast.tsx Outdated Show resolved Hide resolved
airbyte-webapp/src/components/ui/Toast/Toast.module.scss Outdated Show resolved Hide resolved
airbyte-webapp/src/components/ui/Toast/Toast.tsx Outdated Show resolved Hide resolved
@edmundito
Copy link
Contributor

edmundito commented Dec 2, 2022

@dizel852 for the bottom spacing:

Should be 27 px from the bottom instead of 49 as shown in the example Figma:
image

$spacing-page-bottom can be reduced to 88px

@dizel852
Copy link
Contributor Author

dizel852 commented Dec 2, 2022

@dizel852 for the bottom spacing:

Should be 27 px from the bottom instead of 49 as shown in the example Figma: image

$spacing-page-bottom can be reduced to 88px

$spacing-page-bottom - is used to add margin in <MainPageWithScroll /> component. That was a fix for the issue #12780

We can just change the toast bottom margin without changing the $spacing-page-bottom variable.
Since we don't use it in the toast component. 🤷‍♂️

@dizel852
Copy link
Contributor Author

dizel852 commented Dec 2, 2022

Fixed all the comments. Ready for another review.

@timroes
Copy link
Collaborator

timroes commented Dec 2, 2022

@dizel852 I believe we're not using intercom for banners anymore so we could maybe remove the page bottom padding (or make it smaller). Let me check that beginning of next week and get back to you for this.

@dizel852
Copy link
Contributor Author

dizel852 commented Dec 5, 2022

@dizel852 I believe we're not using intercom for banners anymore so we could maybe remove the page bottom padding (or make it smaller). Let me check that beginning of next week and get back to you for this.

😮 Oh, didn't know that. Okay, looking forward to your reply 👍

@dizel852
Copy link
Contributor Author

dizel852 commented Dec 5, 2022

Fixes and updates:

  • need to reserve space for the intercom banner(remain existing 150px) (chatted with @timroes )

Chatted with @Upmitt regarding toast behavior and some cases:

  • align the toast text to left
  • align all elements in the toast to top
  • set max container width to 600px
  • extend the base <Button /> component: do not break the text in the button

Toast behavior examples from @Upmitt:
https://www.figma.com/file/gMNRYVJav25pdsoxAyW7Qp/00_01_WEB-APP-LIBRARIES?node-id=1998%3A3950&t=Z3OLPrNfZvNfbf0o-0

image

Copy link
Contributor

@edmundito edmundito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of minor details 🔍

Copy link
Contributor

@krishnaglick krishnaglick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I have any comments that others have not covered!

@dizel852
Copy link
Contributor Author

dizel852 commented Dec 7, 2022

All comments are resolved. Ready for review.

image

@edmundito
Copy link
Contributor

Tested on chromatic and tried out a few toasts locally.

@dizel852 dizel852 merged commit db70435 into master Dec 8, 2022
@dizel852 dizel852 deleted the vlad/18631-update-toast-design branch December 8, 2022 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to the Airbyte webapp area/platform issues related to the platform team/platform-move
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Toast component design
6 participants