-
Notifications
You must be signed in to change notification settings - Fork 318
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: change box-sizing to have less specificity #1151
Conversation
🦋 Changeset detectedLatest commit: e874036 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@@ -38,8 +38,6 @@ html { | |||
|
|||
// Remove this when this gets fixed in the UI package | |||
* { | |||
box-sizing: border-box; |
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.
This was only affecting the docs site. Removing as it's redundant with the new CSS rule below.
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.
LGTM!
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.
One question about the inclusion of the * selector
all: unset; /* protect against external styles */ | ||
// Note: This rule can be easily overwritten when | ||
// needed due to very low specificity of 0 0 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.
Do we still need this? Taking a look at the original issue, #1092, it sounds like we don't want our styles seeping into other non-amplify ui components. Could this just be removed as all our components have classnames that start with 'amplify'?
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 we remove this, then we will be changing the default box-sizing
behavior of the library for all of the children elements inside of amplify
class components. For example, flex children which are not amplify components would have a different box-sizing
behavior than amplify components.
Without * { box-sizing: border-box; }
<Flex>
<div>1</div> {/* content-box */}
<Button>2</Button> {/* border-box */}
</Flex>
With * { box-sizing: border-box; }
<Flex>
<div>1</div> {/* border-box */}
<Button>2</Button> {/* border-box */}
</Flex>
Leaving it off seems like it would lead to unexpected behavior for customers. What do you think @dbanksdesign
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.
LGTM!
Issue #, if available:
Closes #1092
Description of changes:
This PR fixes an issue where a customer was using a library which injected its CSS too early to override ours. Our
box-sizing
CSS rule had a specificity of0 1 0
. We primarily need this level of specificity for theamplify*
classes, but can reduce the specificity to0 0 0
for other elements. This means any element used in an Amplify UI app will have a defaultbox-sizing
ofborder-box
, but can easily override it with any level of specificity. This should allow other library UI components to be used inside of Amplify UI components such asFlex
without changing theirbox-sizing
styling.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.