Skip to content

Conversation

@wpkelso
Copy link
Member

@wpkelso wpkelso commented May 28, 2025

Rel: feedback on #870

@wpkelso wpkelso requested a review from danirabbit May 29, 2025 02:22

// Overwrite changes made further down the cascade due to the way
// @extend works
padding: 0;
Copy link
Member

Choose a reason for hiding this comment

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

This padding removal seems like an error?

Main:
Screenshot from 2025-06-03 11 42 00
Screenshot from 2025-06-03 11 44 51

This branch:
Screenshot from 2025-06-03 11 42 23
Screenshot from 2025-06-03 11 44 16

Copy link
Member Author

Choose a reason for hiding this comment

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

button.text-button sets a padding of rem(2px) rem(4px), which then gets inherited when we extend it. This is supposed to override that and reset the padding. Before, we got around this by setting the padding for .text-button in a block specific to .text-button, and setting everything else in a shared block.

Compiled as it currently is, the padding is not being overridden. I'm not really sure why, though. Sheets might be cascading in a way I'm not expecting?

image

Copy link
Member

Choose a reason for hiding this comment

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

I see what you're saying. Because we want text buttons to have more horizontal padding and image buttons to have even padding all the way around.

Maybe what we need to do instead then is have a .raised style that just handles the borders and have .linked and .text-button extend that, separately

Copy link
Member Author

Choose a reason for hiding this comment

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

I've split it out using a placeholder class. For some reason the checked and active selectors still had to be overriden

@wpkelso wpkelso requested a review from danirabbit June 6, 2025 19:48
Copy link
Member

@danirabbit danirabbit left a comment

Choose a reason for hiding this comment

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

This looks good to me and I think it makes sense as far as organization goes. Nice job!

@danirabbit danirabbit merged commit b0afe2e into main Jun 28, 2025
5 checks passed
@danirabbit danirabbit deleted the wpkelso/styles/granite-button branch June 28, 2025 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants