fix(Alert): adjust styles when closable is true and inlineActions is set #4435
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Storybook staging is available at https://kiwicom-orbit-sarka-fix-alert-button-overflow.surge.sh |
|
Size Change: +27 B (+0.01%) Total Size: 445 kB
ℹ️ View Unchanged
|
Deploying orbit with
|
| Latest commit: |
d199948
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7c667b7a.orbit.pages.dev |
| Branch Preview URL: | https://sarka-fix-alert-button-overf.orbit.pages.dev |
sarkaaa
force-pushed
the
sarka/fix-alert-button-overflow
branch
from
78b6841 to
af6eb33
Compare
|
I was also thinking about extending this PR by adding visual tests? I've checked respective |
DSil
reviewed
Jul 31, 2024
| @@ -159,6 +159,7 @@ const Alert = (props: Props) => { | |||
| title && inlineActions ? "flex-row" : "flex-col", | |||
| !title && "items-center", | |||
| inlineActions && "justify-between", | |||
| inlineActions && closable && "mr-md", | |||
There was a problem hiding this comment.
This won't work well in RTL. Also, I'm wondering if we can't refactor the current hack we are using for positioning the button (it being absolute with translates is 🤯 )… Can you take a look at that?
And yes, feel free to add more visual tests, for sure!
There was a problem hiding this comment.
This won't work well in RTL.
Should be fixed now.
Also, I'm wondering if we can't refactor the current hack we are using for positioning the button (it being absolute with translates is 🤯 )… Can you take a look at that?
Sure, I have some ideas. On it. 💻
And yes, feel free to add more visual tests, for sure!
OK. 👍
There was a problem hiding this comment.
Change with absolute position removal - 06aa6d3
Please, ignore the min-h-icon-medium classname changes.
- checked with RTL version ✅
- checked without button and multiline content ✅
- mobile view ✅
AlertCloseButton is used only within the Alert component, so the changes shouldn't affect any other component.
Visual tests added ✅
sarkaaa
force-pushed
the
sarka/fix-alert-button-overflow
branch
13 times, most recently
from
a0af89a to
4b8f19c
Compare
|
Before:
Now:
This is not expected. Both the icon and the close button need to stay on top, aligned with the first line of the title (I know the close button was not, so we can either leave in the same position as it was or fix that here) |
sarkaaa
force-pushed
the
sarka/fix-alert-button-overflow
branch
from
4b8f19c to
b9c7e76
Compare
|
I have fixed the position of icon & cancel button. For
WDYT? (before - master branch): Additional note: |
sarkaaa
force-pushed
the
sarka/fix-alert-button-overflow
branch
8 times, most recently
from
c76498c to
618af59
Compare
sarkaaa
force-pushed
the
sarka/fix-alert-button-overflow
branch
from
2974ab6 to
53f09b8
Compare
sarkaaa
commented
Aug 6, 2024
| @@ -145,7 +145,7 @@ const Alert = (props: Props) => { | |||
| <div | |||
| className={cx( | |||
| "me-xs m-0 shrink-0 leading-none", | |||
| inlineActions && "flex items-center", | |||
| inlineActions && "lm:mt-[6px] flex items-center self-baseline", // TODO: [6px] can be replaced by space tokens | |||
There was a problem hiding this comment.
I added margin-top to align the icon with the bottom line of the first row of the title.
I added a comment for the future steps once space tokens are implemented.
DSil
reviewed
Aug 6, 2024
| @@ -164,8 +164,9 @@ const Alert = (props: Props) => { | |||
| {title && ( | |||
| <div | |||
| className={cx( | |||
| "text-ink-dark flex min-h-[20px] items-center font-bold", | |||
| "text-ink-dark min-h-icon-medium flex items-center font-bold", | |||
There was a problem hiding this comment.
Please revert this icon class 🥲
| @@ -74,7 +74,7 @@ const ContentWrapper = ({ | |||
| return ( | |||
| <div | |||
| className={cx( | |||
| "flex min-h-[20px] items-center", | |||
| "min-h-icon-medium flex items-center", | |||
sarkaaa
force-pushed
the
sarka/fix-alert-button-overflow
branch
3 times, most recently
from
0620c08 to
2d83c5b
Compare
sarkaaa
force-pushed
the
sarka/fix-alert-button-overflow
branch
from
2d83c5b to
d199948
Compare
DSil
approved these changes
Aug 7, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This Pull Request meets the following criteria:
For new components:
d.tsfiles and are exported inindex.d.tsCloses https://kiwicom.atlassian.net/browse/FEPLT-2068