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

fix(app): fix the space between icon and banner outline #15082

Closed
wants to merge 11 commits into from

Conversation

koji
Copy link
Contributor

@koji koji commented May 2, 2024

Overview

fix the space between icon and banner outline
Actually, I don't like this fix personally since using margin isn't a good way to make a layout but Banner is used in many components, and we shouldn't touch the component itself since updating the component may cause other bugs.
We will need to update the component to align with the latest design.

design
https://www.figma.com/file/l7BAJAGICr10ELsSqQD0Sr/Design-System%3A-Desktop?node-id=7602-143972&mode=dev

[before]

padding bug on success inline notifcation

[after]

Screenshot 2024-05-02 at 5 25 53 PM

fix RQA-2649

Test Plan

Changelog

Review requests

Risk assessment

@koji koji requested review from ncdiehl11, mjhuff and jerader May 2, 2024 21:38
@koji koji marked this pull request as ready for review May 2, 2024 21:43
@koji koji requested a review from a team as a code owner May 2, 2024 21:43
Copy link
Contributor

@mjhuff mjhuff left a comment

Choose a reason for hiding this comment

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

If we are merging this into edge, and we'll have the time to address broader DQA feedback before next release, do you think it's worthwhile just adding this offset directly to the banner component?

Just looking over all of its usages, I think this margin offset is probably safe to do everywhere. I imagine setting the icon/text a bit more left from the banner's edge will look fine, but if you or others disagree I'm fine with this change for now.

@koji koji changed the base branch from edge to chore_release-7.3.0 May 2, 2024 21:53
@koji koji requested a review from a team as a code owner May 2, 2024 21:53
@koji
Copy link
Contributor Author

koji commented May 2, 2024

If we are merging this into edge, and we'll have the time to address broader DQA feedback before next release, do you think it's worthwhile just adding this offset directly to the banner component?

Just looking over all of its usages, I think this margin offset is probably safe to do everywhere. I imagine setting the icon/text a bit more left from the banner's edge will look fine, but if you or others disagree I'm fine with this change for now.

Actually, this PR should go to chore_release-7.3.0

@mjhuff
Copy link
Contributor

mjhuff commented May 2, 2024

Actually, this PR should go to chore_release-7.3.0

Think you may need to rebase the branch. I think it's bringing all the edge changes into chore-release.

@koji
Copy link
Contributor Author

koji commented May 3, 2024

have failed rebase I will re-open a pr that is based on 7.3.0 since that will be faster than fixing rebase issue.

@koji koji closed this May 3, 2024
@koji koji deleted the fix_RQA-2649 branch May 3, 2024 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants