-
Notifications
You must be signed in to change notification settings - Fork 179
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
fix(app): toast styling fixes #12708
Conversation
Codecov Report
@@ Coverage Diff @@
## edge #12708 +/- ##
==========================================
+ Coverage 73.35% 73.39% +0.04%
==========================================
Files 2261 2264 +3
Lines 61480 61690 +210
Branches 6782 6858 +76
==========================================
+ Hits 45098 45277 +179
- Misses 14763 14773 +10
- Partials 1619 1640 +21
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@@ -215,7 +215,8 @@ export function Toast(props: ToastProps): JSX.Element { | |||
<Icon | |||
name={icon?.name ?? toastStyleByType[type].iconName} | |||
color={toastStyleByType[type].color} | |||
width={showODDStyle ? SPACING.spacing32 : SPACING.spacing16} | |||
maxWidth={showODDStyle ? SPACING.spacing32 : SPACING.spacing16} | |||
minWidth={showODDStyle ? SPACING.spacing32 : SPACING.spacing16} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess using the same value for min/max is the same as the previous code.
Do they behave differently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surprisingly, they do. Long text would squish the icon, shrinking it to just a few pixels wide. Rather than figuring out the max length of the text area for both desktop and ODD and enforcing it there, it was cleaner to just lock the icon size by setting the min and max size.
5842d82
to
087e367
Compare
maybe including the style updating for a long name case too? Screen.Recording.2023-05-16.at.5.12.34.PM.mov |
Thanks for posting that! I couldn't reproduce this in storybook, so it's good to have a real-world example. |
@koji Found the width issue in the ToasterOven and fixed it there. |
Overview
This PR fixes text overflow and alignment issues in the desktop and ODD toasts
Closes both RAUT-412 and RAUT-379
Test Plan
Manually verified close text placement and styling and long message overflow truncation in storybook
Changelog
Review requests
In storybook, set the toast message and optional header to be absurdly long. Verify it is truncated and not stretching the toast off the edge of the screen. Verify the close text is centered vertically. Verify close text is underlined on desktop.
Risk assessment
None.