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(alert): ensure border-radius is consistent for prescribed slots #6368

Conversation

Elijbet
Copy link
Contributor

@Elijbet Elijbet commented Jan 27, 2023

Related Issue: #6348

Summary

The bottom left corner radius was missing when the icon was present. Upon inspection, it was clear that slots added around content don't have the proper radius set and end up covering the radius on the host. For each occasion the slot is placed against either end of the host, I adjusted the relevant radius to match the host container.

Because the close button is now always present, the cases addressed are the right side of the close button and the left side of the content (no icon) along with the left side of the icon (when placed in front of the content).

Added a screenshot for the radius with options where icon is present and absent.

@Elijbet Elijbet requested a review from a team as a code owner January 27, 2023 22:08
@github-actions github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Jan 27, 2023
Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Other than a small comment, this LGTM! 🚀

@@ -398,7 +398,7 @@ export class Alert implements OpenCloseComponent, LoadableComponent, T9nComponen
@State() queued = false;

/** the close button element */
private closeButton?: HTMLButtonElement;
Copy link
Member

Choose a reason for hiding this comment

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

This is fine for now, but for the future, let's keep these cleanup changes separate unless the code specific PR changes require a refactor or if it's in the modified line vicinity. Reasoning: there are no additional changes here that relevant to the PR.

@jcfranco
Copy link
Member

@Elijbet I forgot to add, can you revisit the PR title to summarize the changes from a consumer perspective (i.e., what does this fix for users)?

@Elijbet Elijbet changed the title fix(alert): border radius fix(alert): improves visual consistency of the component by fixing the application of the border-radius Jan 27, 2023
@Elijbet Elijbet added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Jan 27, 2023
@Elijbet Elijbet changed the title fix(alert): improves visual consistency of the component by fixing the application of the border-radius fix(alert): border-radius was fixed Jan 27, 2023
@Elijbet Elijbet changed the title fix(alert): border-radius was fixed fix(alert): ensure border-radius is consistent for prescribed slots Jan 28, 2023
@Elijbet Elijbet changed the title fix(alert): ensure border-radius is consistent for prescribed slots fix(alert): ensure border-radius is consistent for prescribed slots Jan 28, 2023
@Elijbet Elijbet added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jan 28, 2023
@Elijbet Elijbet merged commit cfe5699 into master Jan 28, 2023
@Elijbet Elijbet deleted the elijbet/6348-alert-south-west-corner-radius-missing-when-icon-present branch January 28, 2023 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug reports for broken functionality. Issues should include a reproduction of the bug. pr ready for visual snapshots Adding this label will run visual snapshot testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants