-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Make sure toast max width doesn't exceed screen size on mobile devices #6959
Conversation
Generate changelog in
|
Make sure toast max width doesnt exceed screen size on mobile devicesBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
@@ -103,7 +103,7 @@ $toast-intent-colors: ( | |||
box-shadow: $pt-toast-box-shadow; | |||
display: flex; | |||
margin: $toast-margin 0 0; | |||
max-width: $toast-max-width; | |||
max-width: min($toast-max-width, calc(100vw - $toast-margin)); |
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.
probably should handle considering $toast-min-width
here as well 🤔
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.
from https://www.w3.org/TR/CSS21/visudet.html#min-max-widths
The following algorithm describes how the two properties influence the used value of the 'width' property:
The tentative used width is calculated (without 'min-width' and 'max-width') following the rules under "Calculating widths and margins" above.
If the tentative used width is greater than 'max-width', the rules above are applied again, but this time using the computed value of 'max-width' as the computed value for 'width'.
If the resulting width is smaller than 'min-width', the rules above are applied again, but this time using the value of 'min-width' as the computed value for 'width'.
so I guess I should do something similar to this for the min-width
field as well (though we're propobably safe with 300px...)
Update packages/core/src/components/toast/_toast.scssBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Co-authored-by: Greg Douglas <gdouglas@palantir.com>
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.
LGTM!
Update packages/core/src/components/toast/_toast.scssBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Invalidated by push of 33cc203
Update packages/core/src/components/toast/_toast.scssBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Fixes no existing issue: https://github.com/palantir/blueprint/issues?q=is%3Aissue+is%3Aopen+toast
Checklist
I could try to update the tests to avoid regressing but think this may be pretty hard to test for, particularly given how long this went without being a big issue
Changes proposed in this pull request:
Modify toast "max-width" to be the minimum of page width - 20px and 500px, rather than the max width always being 500px which may exceed the screen size, particularly on mobile devices.
Reviewers should focus on:
any concerns/ possible negative impact from this change
Screenshot
this PR
layout gets messed up but I think this is an improvement over current behavior, particularly for toasts that don't have an action button
current