Clean BlockMover component and styles #40379
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
youknowriad
added
the
[Type] Code Quality
|
Size Change: -964 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
|
The amount of retired code in this branch is delightful, thank you for refactoring this stuff, it'll pay dividends. Some of the metrics are wrong. They are wrong in trunk already compared to the ideal setup as outlined here, but nevertheless. Here's a gif showing smaller mover control sizes in trunk:
Here's a gif showing that at least the block type indicator is now 48x48 even though it should ideally be 36x48: I can dive into a the code next week, my day is close towards ending, but generally the codepen still holds up. Every toolbar segment has 6px padding left and right, and every button inside is 36px wide at most. The only exception is the drag handle, which is 24px wide. I updated the codepen to show the mover control configurations:
Thanks again for working on this 🏅 |
|
Thanks for the codepen it helps, but the codepen is about the whole toolbar while the BlockMover is also a reusable component on its own. So I'd say, I need another codepen with just the block movers to be able to make the right implementation :) (the Block icon/switcher is not part of the mover) |
|
The other thing that is not present is the "mobile". Right now, we show the buttons are regular buttons (we remove the special treatment for the arrows). I wonder if we want to keep that. |
|
So I've looked again at the code pen. And it seems that in the context of the block toolbar, this PR's only remaining change is that the arrows width should be 36 instead of 48. (I guess only on desktop since on mobile the buttons stack anyway). Also, it's not clear whether the block mover component (see screenshots on my PR description) should be 48 or 36 by default. |
|
Ok. I pushed more tweaks here. Let me know if there's anything left. That said, this is slightly more impactful with the last commit. (I moved the toolbar treatment to buttons to the generic toolbar component instead, no need for overrides). |
youknowriad
commented
Apr 15, 2022
| @@ -13,6 +15,24 @@ | |||
| } | |||
|
|
|||
| line-height: 0; | |||
|
|
|||
| // Size multiple sequential buttons to be optically balanced. | |||
| // Icons are 36px, as set by a 24px icon and 12px padding. | |||
There was a problem hiding this comment.
This is now a behavior that is applied by default to all toolbars.
jorgefilipecosta
approved these changes
Apr 15, 2022
| .components-button, | ||
| .components-button.has-icon.has-icon { | ||
| min-width: $block-toolbar-height - $grid-unit-15; | ||
| padding-left: $grid-unit-15 * 0.5; // 6px. |
There was a problem hiding this comment.
Would it make sense to specify this as ($button-size - $icon-size) *0.5?
There was a problem hiding this comment.
I have no idea 😬 This is based on @jasmussen's computations
There was a problem hiding this comment.
We use 6px in a bunch of places, and in most of those places we use them based on the 12px grid, which is $grid-unit-15. Hence the particular math here.
One thing we could look at is to move towards a base4 grid system instead of a base8 grid system, but this is kind of a big shift, and perhaps something to consider more for the new component system.
There was a problem hiding this comment.
Changes around here lowered the specificity of the rules for the button component, and I have a small followup to fix it in #40438.
|
With the latest changes, this looks amazing to me, except for in two places, both related to locking. Labelled toolbars look good: Labelled locked toolbars, and just plain locked toolbars look wrong:
Note that this part of the code seems a bit sensitive, I had to apply some bandaids recently in #40037 — I wonder if removing some of those bandaids will fix it up? |
|
I think I fixed those locked switcher issues. |
|
I think the click area for horizontal movers is a bit small now. I can make them bigger but it means the "vertical" and "horizontal" movers won't have the same width. Maybe that's fine. |
|
I made the horizontal buttons a bit bigger in the last commit. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
While working on #40314 I noticed that the BlockMover component is hard to reuse as is. You need to override several styles. The current PR tries to clean its state as a reusable component, adds a storybook to be able to design it and build it out of context and also tries to make sure all the current behavior stays as is.
Why?
Mostly a code quality and maintainability change.
How?
Sometimes the best way is to refactor is to start clean. I left the component as is (minor changes) but I rewrote the CSS from scratch. I've used the storybook first to make sure the component is working as it's supposed to out of context and then adapted it to context later (block toolbar...)
Testing Instructions
Screenshots or screencast