feat(toast): toast vertical align prop #6391
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit ab8c768:
|
nuria1110
force-pushed
the
FE-6209-toast-screen-position
branch
from
bcf0d7a to
fb57583
Compare
There was a problem hiding this comment.
Some minor comments from me but overall great work @nuria1110 👏 .
I think we could also slightly tweak the commit message here as it will form the release note when this is merged. Something like below might work but happy for you to go with what you want here 😄
feat(toast): add alignY prop to allow vertical alignment to be configurable
adds `alignY` prop to `Toast` which allows consumers to vertically align the component on the screen
fix #6301
| @@ -234,6 +234,164 @@ Visual.parameters = { | |||
| themeProvider: { chromatic: { theme: "sage" } }, | |||
| }; | |||
|
|
|||
| export const AllAlign = () => { | |||
There was a problem hiding this comment.
praise: nice idea to use chromatic to check all the align permutations 👍🏼
src/components/toast/toast.spec.tsx
Outdated
| }); | ||
|
|
||
| it.each(["top", "center", "bottom"] as const)( | ||
| "should then pass the prop to Styled Portal", |
There was a problem hiding this comment.
suggestion(non-blocking): it might help rewording this slightly so its a bit clearer what the preconditions are:
| "should then pass the prop to Styled Portal", | |
| "when align prop is %s, Portal is correctly positioned", |
| toastComponent() | ||
| .parent() | ||
| .parent() | ||
| .parent() | ||
| .parent() |
There was a problem hiding this comment.
thought: it might be worth when we migrate these tests to Playwright to amend how we are locating the portal element here.
It's not essential now, but if we can directly locate the portal element instead of traversing the DOM, this should make the test less fragile if we ever change the component's markup.
nuria1110
force-pushed
the
FE-6209-toast-screen-position
branch
from
befad0f to
8396b43
Compare
|
Approved PR but wasnt sure we wanted centre aligned, however we are considering some updates around that placement, so good to allow that to be included and we can test. |
nuria1110
force-pushed
the
FE-6209-toast-screen-position
branch
from
ecaf08a to
47bfe28
Compare
…urable adds `alignY` prop to Toast which allows consumers to vertically align the component on the screen fix #6301
nuria1110
force-pushed
the
FE-6209-toast-screen-position
branch
from
47bfe28 to
64203e1
Compare
|
🎉 This PR is included in version 123.6.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
fix #6301
Proposed behaviour
alignYprop which allows to vertically align the component at the top, center and bottom of the screen.noticevariant will still render at the bottom of the screen by default but you can setalignYset to "top" to render it at the top of the screen.alignwas set to left.Current behaviour
alignis set to "left" and the screen size is small:Checklist
d.tsfile added or updated if requiredQA
Additional context
Testing instructions
Added a test story for cases where there is more than one toast on the screen at the same time and have different vertical alignments.
The following CodeSandbox is an example of the broken behaviour.
You can see the new behaviour by looking at the version in the comment by
codesandbox[bot].