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.

Sign up for GitHub

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

Jump to bottom

Add toast positioning #32280

Merged
merged 4 commits into from
Dec 4, 2020
Merged

Add toast positioning #32280

merged 4 commits into from
Dec 4, 2020

Conversation

MartijnCuppens
Copy link
Member

@MartijnCuppens MartijnCuppens commented Nov 29, 2020 •
edited
Loading

Alternative for #28783. Uses our built-in spacing utilities doesn't require JavaScript positioning.

Also added translate-middle-x & translate-middle-y utilities for more positioning possibilities.

Preview:

Closes #28783

@MartijnCuppens MartijnCuppens requested review from a team as code owners November 29, 2020 15:04
@XhmikosR
Copy link
Member

XhmikosR commented Nov 29, 2020 •
edited
Loading

From a really quick look, I definitely dig it ❤️

Needs more eyes from @twbs/css-review

EDIT: if we are going to go with this approach, I'm not sure it makes sense to merge #28783 in beta1.

@XhmikosR
Copy link
Member

BTW the center widgets look weird on mobile

image

@MartijnCuppens MartijnCuppens force-pushed the main-mc-toast-positioning branch 2 times, most recently from b6a3c7d to f9e2c80 Compare November 29, 2020 19:37
@MartijnCuppens
Copy link
Member Author

BTW the center widgets look weird on mobile

Fixed that

mdo
mdo approved these changes Nov 29, 2020
View reviewed changes
Copy link
Member

@mdo mdo left a comment

Choose a reason for hiding this comment

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

Let's do it!

@XhmikosR
Copy link
Member

@mdo we have #28783 included in beta1. I'm not sure if these are 1 to 1 equivalent, but if so, I guess this one should be merged and not #28783.

@mdo
Copy link
Member

mdo commented Dec 2, 2020 •
edited
Loading

Widths need to be ironed out I think... by default, the toasts are shrunk here. Is that behavior we want overall? Would we want to change to this behavior at all times and let folks pick their own max-widths?

Screen Shot 2020-12-02 at 10 28 00 AM

Screen Shot 2020-12-02 at 10 24 15 AM

It's not until I add wrapping content in one that they hit their max-width that's set on the .toast.

Screen Shot 2020-12-02 at 10 22 33 AM

@mdo mdo mentioned this pull request Dec 2, 2020
Closed
7 tasks
mdo
mdo reviewed Dec 2, 2020
View reviewed changes
scss/_toasts.scss Show resolved Hide resolved
@mdo
Copy link
Member

mdo commented Dec 2, 2020

Also, could this arguably wait until another release? Not sure we need to lock in something for the beta release.

@MartijnCuppens MartijnCuppens force-pushed the main-mc-toast-positioning branch 2 times, most recently from 1b50bc0 to a79043b Compare December 3, 2020 20:54
@MartijnCuppens
Copy link
Member Author

Good point, hadn't noticed that. We'll need the width: max-content; to fix #32280 (comment)

Just fixed the shrinking thing by using margins between the toasts instead of the using grid.

mdo
mdo approved these changes Dec 3, 2020
View reviewed changes
Copy link
Member

@mdo mdo left a comment

Choose a reason for hiding this comment

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

Thank you!

@XhmikosR
Copy link
Member

XhmikosR commented Dec 4, 2020 •
edited
Loading

Branch rebased and replaced the renamed classes.

@ffoodd RTL-wise does this LGTY? Also, does cheatsheet needs to be updated?

ffoodd
ffoodd approved these changes Dec 4, 2020
View reviewed changes
Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

LGTM!

Since position relies on our utilities, that's already handled and processed by RTLCSS 👍

Regarding cheatsheet, I don't think so but that soemthing to question: I didn't show utilities in the cheatsheet for now. I'll add it to our remaining to-do.

@XhmikosR XhmikosR changed the title Toast positioning Add toast positioning Dec 4, 2020
@XhmikosR XhmikosR merged commit 5f89ea3 into main Dec 4, 2020
@XhmikosR XhmikosR deleted the main-mc-toast-positioning branch December 4, 2020 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XhmikosR XhmikosR XhmikosR left review comments

@ffoodd ffoodd ffoodd approved these changes

@mdo mdo mdo approved these changes

Assignees
No one assigned
Labels
css feature no squash no-backport-to-v4 v5
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MartijnCuppens @XhmikosR @mdo @ffoodd