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

Update Alert banner message-body to match design system #9463

Merged
merged 2 commits into from
Jul 14, 2020

Conversation

Monkeychip
Copy link
Contributor

The new design system has the alert banner text black. This PR updates that.

To have the least impact on other elements/components using the .message-body class I created a new .scss file for alert-banners and renamed the class it's using. This keeps the change contained to the alert-banner component.

Here are the design system pictures:

Here is the before the fix:

Here is the fix with the PR:

@Monkeychip Monkeychip added the ui label Jul 13, 2020
@Monkeychip Monkeychip added this to the 1.5 milestone Jul 13, 2020
Copy link
Contributor

@chelshaw chelshaw left a comment

Choose a reason for hiding this comment

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

I really like the way you approached this (creating a new scss file for the component). Just one small comment, but looks great!

@@ -0,0 +1,6 @@
.alert-banner-message-body {
border: 0;
margin-top: $spacing-xxs;
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like the border and margin-top here have the same values as the original message-body class. in that case, what do you think about extending message-body so these styles aren't duplicated? this way if we make any other styling changes to all messages (perhaps a new font or font weight, etc), they'll apply here too. i.e.:

.alert-banner-message-body {
@extend .message-body
color: $black;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that's a great idea. I didn't know you could do that. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@noelledaley ... um. 🤔 this doesn't seem to work because message-body color is overriding the black here. I would have to use !important after black to make it work, and that doesn't seem like an optimal solution.

@Monkeychip Monkeychip merged commit 68ecbf8 into master Jul 14, 2020
Monkeychip added a commit that referenced this pull request Jul 14, 2020
* make alert banner text black

* remove comment in scss file
Monkeychip added a commit that referenced this pull request Jul 14, 2020
* make alert banner text black

* remove comment in scss file
@andaley andaley deleted the ui/alert-banner-css-fix branch July 20, 2020 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants