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.

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

[MISC][NT] Aligned icons and copy for alert component with icon variant #604

Merged
merged 3 commits into from
Nov 4, 2024

Conversation

nobeltws
Copy link

Changes
Fixed alignment of alert component to center contents when icon is rendered

  • [delete] branch

Changelog entry

  • [WARNING] Center contents when icon is rendered in Alert

  • You may refer to this BOOKINGSG-6398

@nobeltws nobeltws requested a review from qroll October 25, 2024 09:49
@qroll
Copy link
Contributor

qroll commented Oct 25, 2024

actually the icon will need to be top aligned (Figma)

Screenshot 2024-10-25 at 6 52 26 PM

currently it is vertically centered when text wraps
Screenshot 2024-10-25 at 6 59 37 PM

could you try setting a fixed height on the AlertIconWrapper?

@qroll qroll added the bug Something isn't working label Oct 25, 2024
@qroll qroll added this to the v2.8.0-canary.5 milestone Oct 25, 2024
@nobeltws
Copy link
Author

nobeltws commented Oct 25, 2024

oops, my bad. i removed the align-items: center css from the AlertIconWrapper. Seems no need to set the height
for single line it is now like this:
image

With max collapse height:
image

@qroll
Copy link
Contributor

qroll commented Oct 29, 2024

annoyingly, now the small variant becomes misaligned, probably due to the different text height on the font

Screenshot 2024-10-29 at 12 27 09 PM

what if we remove the custom height height: 1.625rem;, and apply height: 1lh and align-items: center on AlertIconWrapper? this forces it to match the height of the first line

@nobeltws
Copy link
Author

nobeltws commented Nov 4, 2024

Updated with your suggestions. i checked in storybook, looks good~

Copy link
Contributor

@qroll qroll left a comment

Choose a reason for hiding this comment

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

thanks for the fix!

@qroll qroll merged commit aa8a7ee into master Nov 4, 2024
1 check passed
@qroll qroll deleted the align-alert-contents branch November 4, 2024 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants