-
Notifications
You must be signed in to change notification settings - Fork 147
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: Remove randomized css widths #635
Conversation
Signed-off-by: Tamika Tannis <[email protected]>
Signed-off-by: Tamika Tannis <[email protected]>
@@ -4,7 +4,7 @@ | |||
$shimmer-loader-items: 1, 2, 3, 4, 5; | |||
$shimmer-loader-row-height: 16px; | |||
$shimmer-loader-row-min-width: 90; |
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.
You could remove this one too
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.
$shimmer-loader-row-min-width
still used on line 79 for now, do you want to remove that too?
amundsen_application/static/js/components/common/ShimmeringTagListLoader/styles.scss
Outdated
Show resolved
Hide resolved
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
amundsen_application/static/js/components/common/ShimmeringTagListLoader/styles.scss
Outdated
Show resolved
Hide resolved
Signed-off-by: Tamika Tannis <[email protected]>
ab0464f
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
Summary of Changes
Using randomized widths negates the effects of css content hashing, as the built content will be different each time due to the random widths. This PR removes those usages, and for now I have just chosen
75%
for widths that need a percentage, and splitting the different between the old max width and min width for others.Tests
N/A but confirmed the build returns the same
contenthash
each time.Documentation
N/A
CheckList
Make sure you have checked all steps below to ensure a timely review.