Skip to content

Add toast positioning#32280

Merged
XhmikosR merged 4 commits into
mainfrom
main-mc-toast-positioning
Dec 4, 2020
Merged

Add toast positioning#32280
XhmikosR merged 4 commits into
mainfrom
main-mc-toast-positioning

Conversation

@MartijnCuppens
Copy link
Copy Markdown
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
@MartijnCuppens MartijnCuppens force-pushed the main-mc-toast-positioning branch from a569976 to 445f8af Compare November 29, 2020 15:06
XhmikosR
XhmikosR reviewed Nov 29, 2020
View reviewed changes
Comment thread site/assets/js/application.js
@XhmikosR
Copy link
Copy Markdown
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
XhmikosR reviewed Nov 29, 2020
View reviewed changes
Comment thread site/assets/js/application.js
@MartijnCuppens MartijnCuppens force-pushed the main-mc-toast-positioning branch from 445f8af to c93ee96 Compare November 29, 2020 18:07
@XhmikosR
Copy link
Copy Markdown
Member

XhmikosR commented Nov 29, 2020

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
Copy Markdown
Member Author

MartijnCuppens commented Nov 29, 2020

BTW the center widgets look weird on mobile

Fixed that

mdo
mdo approved these changes Nov 29, 2020
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

XhmikosR commented Nov 30, 2020

@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.

@MartijnCuppens MartijnCuppens force-pushed the main-mc-toast-positioning branch from f9e2c80 to d4a1736 Compare November 30, 2020 18:01
@mdo
Copy link
Copy Markdown
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
@XhmikosR XhmikosR force-pushed the main-mc-toast-positioning branch from 1d9c825 to 2cc726b Compare December 2, 2020 18:33
mdo
mdo reviewed Dec 2, 2020
View reviewed changes
Comment thread scss/_toasts.scss
@mdo
Copy link
Copy Markdown
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
Copy Markdown
Member Author

MartijnCuppens commented Dec 3, 2020

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
Copy Markdown
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 XhmikosR force-pushed the main-mc-toast-positioning branch from a79043b to a9c1114 Compare December 4, 2020 06:26
@XhmikosR
Copy link
Copy Markdown
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
Copy Markdown
Contributor

@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 force-pushed the main-mc-toast-positioning branch from 428f736 to f138a2a Compare December 4, 2020 14:12
@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

@mdo mdo mdo approved these changes

+1 more reviewer

@ffoodd ffoodd ffoodd approved these changes

Reviewers whose approvals may not affect merge requirements

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