-
Notifications
You must be signed in to change notification settings - Fork 60
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: added self-contained styles for alert component #123
Conversation
Deploy preview for fundamental-styles ready! Built with commit 02b4f50 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there are multiple other CSS variables you could put into the ugly theme. Check the scss/components/alert.scss
file for other uses.
components/alert.scss
Outdated
@import "../scss/components/alert"; | ||
@import "../scss/components/link"; | ||
@import "../scss/core/elements"; | ||
@import "../scss/core/root"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Importing the entire core elements and root files is not what we want to do here. We need to find out what from these files is necessary to render alerts properly and bring those into the link class definitions -- either via common mixins or directly. The whole point of self-contained styles is not only bring along what is needed and not do the changes using elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a number of visual regression file changes. Is it possible to see what the differences are between the old and the new?
scss/components/alert.scss
Outdated
p { | ||
margin-bottom: 0; | ||
margin-top: 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Styles cannot be applied directly to elements (<p>
in this case) as they can bleed into other components on the page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@greg-a-smith Thank you for the review, will create a class for it to prevent the bleeding.
scss/mixins/_mixins.scss
Outdated
&:focus { | ||
outline-style: dotted; | ||
outline-width: 1px; | ||
@include fd-var-color("outline-color", fd-color-state("hover", "action"), --fd-color-action-focus); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If all the changes in this file are needed, I would expect these to be part of the link component's PR. Once they are there, this PR can take advantage of those same styles.
…to fix/self-contained-alert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming the build passes, this looks good. 🚢
Closes #97
This PR contains the self-contained styling for the alert component.
This PR is part of the larger task of making all components self-contained #136