Skip to content

Conversation

@dancormier
Copy link
Contributor

@dancormier dancormier commented Dec 5, 2022

The recent refactor of button styling introduced a bug where an inner box shadow was set on the base button styling. This was an unintended change and, since these buttons featured a transparent background, the box shadow would create a rather unpleasant style.

image
image

This PR fixes this styling bug. Once merged, I'll cut a new patch release.


Thanks for the report @abovedave! I've added you as a reviewer for this. Can you take a peek at the deploy preview and let me know if the box shadows render for you as expected?

@dancormier dancormier added the bug A reproducible problem with the Stacks code label Dec 5, 2022
@dancormier dancormier requested a review from abovedave December 5, 2022 16:56
@netlify
Copy link

netlify bot commented Dec 5, 2022

Deploy Preview for stacks ready!

Name Link
🔨 Latest commit c75943a
🔍 Latest deploy log https://app.netlify.com/sites/stacks/deploys/638e22a4b4fc530009121f14
😎 Deploy Preview https://deploy-preview-1203--stacks.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Collaborator

@abovedave abovedave left a comment

Choose a reason for hiding this comment

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

Fixed the base button, but I'm wondering if it needs moving into the &.s-btn__filled modifier for danger and muted as well @dancormier?

--_bu-bs: inset 0 var(--su-static1) 0 0 hsla(0, 0%, 100%, 0.4);

--_bu-bs: inset 0 var(--su-static1) 0 0 hsla(0, 0%, 100%, 0.4);

@abovedave
Copy link
Collaborator

abovedave commented Dec 5, 2022

Should the alpha for those be 0.7 instead of 0.4 as well?

@dancormier
Copy link
Contributor Author

dancormier commented Dec 5, 2022

Should the alpha for those be 0.7 instead of 0.4 as well?

My goal with the button CSS refactor was to maintain styles as-is while cleaning up the structure and cascade. The box shadow for the base filled button has 0.7 as the alpha value, while the danger and muted filled variants both use 0.4, so I've left them in tact. I'm assuming this was intended to unify the look of that box shadow across button variants (since the base style background color is lighter and requires a more opaque shadow to contrast with it) but I'm not 100% sure of the reasoning.

@dancormier dancormier merged commit f79b666 into develop Dec 5, 2022
@dancormier dancormier deleted the dcormier/fix-btn-inner-box-shadow branch December 5, 2022 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug A reproducible problem with the Stacks code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants