Audit variables stylesheet. #26827
Audit variables stylesheet. #26827
Conversation
|
Note, if you check out this PR, you have to stop and restart |
|
Size Change: -68 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
This worked fine for me, I didn't notice any visual differences or issues. Code-wise all the changes makes sense and improvements.
I don't have much opinions on the CSS, so can't offer much input if you are looking for more, otherwise feel free to merge once the conflict is resolved. 👍
jasmussen
force-pushed
the
try/variable-audit
branch
from
1c78f5c
to
b7449a6
Compare
|
Thanks for the native side commit Joen! Has the variable been replaced with something else that we should start using on mobile? Reeling in @lukewalczak too that started using the |
Thank you for looking, thank you for the ping. To provide a proper answer, I have to go over the history for a bit. In WordPress 5.4 and earlier, blocks used to have 1px gray borders all around them. These borders were outset 14px, meaning there had to be at least 28px margin between two blocks in order for these borders to not overlap when multiple blocks were selected. An additional 4px margin between those two blocks were added, so that the two gray borders wouldn't be flush against each other when multi selected. Those 4px were stored in the $block-spacing variable. All these borders, paddings, and margins made the UI math pretty tricky, which is why the variables were stored in the global stylesheet — they were simply used across enough files that it was the right place to store them. As of 5.5, the block UI was substantially reduced — instead of a gray outset border shown when the block is selected, a proper focus style is shown, which is painted on the exact footprint of the block, not outset. While we still have a default margin between blocks ( In other words, the I know that mobile native is a slightly different beast, with a longer path towards WYSIWYG. So if you have need of a variable to replace this one, it depends a bit on what it is to be used for. In context, I see this: It doesn't look like the block spacing is being used to vertically space out the blocks here, so it looks like you need a variable that may be unique to mobile native. If it is to be used across multiple files, feel free to create a new one, there are a couple that are prefixed |
|
Forgot to ask: would you mind if I merge this one, @hypest? Happy to address any comments you have. |
|
Hey @jasmussen, thanks for that PR!
You're totally right. I believe that this variable was used because the value of it fits to design requirements (which is kinda wrong usage) I think that the grid variable ( For the button, I would go with the local variable since it is specific to that block. |
|
Thank you @dratwas! I do not have my local system set up to compile and test for mobile native. If you do, and have time/energy, feel free to push directly to this branch to make any changes. Otherwise, I can follow up with a PR, though I will be unable to test that PR and would need to trust you to review it. What works best? |
|
Let me change the style in the file block to the Thanks |
There was a problem hiding this comment.
Tested it on the mobile side and didn't notice any visual differences or issues.
Thank you @jasmussen for this!
|
Excellent 👌 I'll merge when the checks pass. Thanks for testing! |
|
There's this instance of the variable also: — do we keep that there for now? |
|
Yes, as I mentioned |
|
Cool, I'll land this! |
We have a handy stylesheet,
_variables.scss, which provides a number of base styles. These variables can be accessed across the codebase, making it easy to update visuals across the entire system, when necessary, by changing a single variable.However the sheet had a few problems:
This is primarily a code quality PR, but it also hopes to address those problems in a few ways:
If you test this PR, there shouldn't be any visual changes, except in two places, that now use grid units rather than unrelated variables: