Full Height Alignment Toolbar: Implementation + Cover integration. #26615
Conversation
retrofox
added
[Type] Enhancement
retrofox
requested review from
ajitbohra,
ellatrix,
Soean and
talldan
as code owners
|
Size Change: +334 B (0%) Total Size: 1.2 MB
ℹ️ View Unchanged
|
retrofox
force-pushed
the
update/add-full-height-alignment-toolbar
branch
from
44c8e93
to
066ad89
Compare
|
Thank you for working on this. This is what I see:
Since we last discussed this feature, I've had a chance to think a bit more about how it might behave. Specifically, we might want to have the toolbar button for just a few blocks, but instead of applying a classname (
The reason being that height is a little different from width in a web that is mostly a vertical flow of content. For width, it might make sense to have specific width presets (and indeed allow these on all block, #25973), which could look like this in the sidebar:
But height should probably remain flexible and fluid, and have an input field. It could probably still have a toolbar button, like this PR adds, but it should only be applied to a few blocks. But the design is a bit tricky. Here's an attempt at a better icon:
SVG: I know the removal of the classname is a bit of a change from the initial discussions. I'd be curious for thoughts by @jameskoster, @ItsJonQ and @mtias |
|
Having a toolbar button to quickly set the height to 100vh seems like a handy shortcut, but I would agree it's probably best restricted to just a few blocks. It may be something to discuss in another issue, but I find the alignment icons themselves quite tricky to understand, particularly wide and full width. If we're going to add an icon for full height, I wonder if we might revisit and attempt to find consistency across all these alignment icons? |
|
Thanks, @jasmussen for your review
Sorry, do you see something wrong there? Just in case, the full height is applied to the block, not to the image component.
If you'd like to stretch full height, you should enable
This PR adds
Sounds good to me. Going to work on this.
Looks great to me. Just let's keep in mind that I think it worths refactoring the current implementation before starting to iterate over this since if I'm not wrong, this component is defined in the
👍
It's added to only two blocks :-D
Excellent. |
|
Thanks, James.
Yes, agree :-D
The reason why I've put it in a different section of the toolbar is that the most powerful feature of this ( seemingly simple) alignment is the combination with other alignment types. Just FYI, we've discussed a lot in the previous PR. |
Absolutely. I was having the same thought notably with the Media & Text toolbar, which becomes very abstract with the black bar icons.
No no, the feature is working fine! The GIF was perhaps misleading because the image was too low resolution to fill the vertical height.
Yes indeed. For starters, though, I think the approach to agree on is whether we should remove the full-height classname and replace it with an inline style. I think that could be a good first step to unifying the dimensions. |
It already applies the height using inline styles in the |
|
Updated icon here e73765c
|
Just to be clear, it applies inline style right now in this PR :-D. I've written a section about this in the PR description. |
Thank you for the clarification, definitely worth looking closely at the bottom of your PR description 🙈 — I blame this week being unusually exhausting. What I'm saying though, is that we need to not have the CSS class at all:
The way I see it, this should only be two controls:
Those two could/should play together as you do now, insofar as if the height is set to 100vh, the button is toggled. Is that possible to do? |
Let me clean the
:-) No worries.
Let me clean the
Already implemented in the cover component, correct?
Already implemented in this PR.
It is at all! and this is something that I did before but... (always there is "but") there is a minor issue here that made me add the toggle in the sidebar (I tried to explain it in the PR too 😅)
does it make sense of this concern for you? |
Keep in mind that unit control applies
Just saying because this small difference could be an issue if we aren't totally aware. |
|
Yep, that's a good differentiation, thank you for the clarification. It does seem like we'll only ever want one field for height, if we can do with it, whether that's min-height or plain height. I think I understand the problem, and I also think I need to give my brain a rest to think more about it 😅 The difficult part here is getting the interface right! |
|
In terms of functionality, if we don't apply
Applying |
|
Toggle
Toggle active state of
|
|
This is getting there:
I would say the user experience is now very close:
Markup also looks good:
This is growing on me, but because it touches both the cover block and the min-height values, this needs a little sanity checking. Here are a few that have worked on related properties: @sirreal @nosolosw @jorgefilipecosta — thanks for checking! Visually, I would like to see a small change to the toolbar as it looks here:
Can we do this instead:
That is — the fullscreen button feels like it belongs with the other alignments. |
retrofox
force-pushed
the
update/add-full-height-alignment-toolbar
branch
from
f0b85e1
to
0c3c30e
Compare
yeah... I agree honestly, but I liked to play with the dynamic label. Removed ✔️ |
retrofox
force-pushed
the
update/add-full-height-alignment-toolbar
branch
2 times, most recently
from
523b32f
to
d2cc739
Compare
retrofox
changed the title
packages/block-editor/src/components/block-full-height-alignment-toolbar/icon.js
Outdated
Show resolved
Hide resolved
| @@ -264,6 +266,47 @@ function CoverEdit( { | |||
| const onSelectMedia = attributesFromMedia( setAttributes ); | |||
| const isBlogUrl = isBlobURL( url ); | |||
|
|
|||
| const [ prevMinHeightValue, setPrevMinHeightValue ] = useState( minHeight ); | |||
There was a problem hiding this comment.
I guess these two state values could share the same useState call the value being an object with properties minHeight and minHeightUnit that could be directly passed to set attributes. These change also aligns with probable change we will have to do make minHeightUnit and minHeight a single attribute (a string that can contain multiple CSS valid properties).
If we don't initiate the state of previous to minHeight, minHeightUnit e.g: keep them undefined and also follow the suggestion of setting the previous values inside toggleMinFullHeight we can also probably remove the need for this condition bellow "// If there aren't previous values, take the default ones.".
There was a problem hiding this comment.
Nice suggestion. Going to refactoring using an object.
These change also aligns with probable change we will have to do make minHeightUnit and minHeight a single attribute (a string that can contain multiple CSS valid properties).
💯 agree.
I did this approach in the Background Size implementation. The data is stored in the backgroundSize attribute (string)ready to be used in the inline style. The block takes over of parsing and converting from/to this attribute to get the data usable by the UI components.
Happy to do this in a follow-up PR.
retrofox
force-pushed
the
update/add-full-height-alignment-toolbar
branch
from
d2cc739
to
7168e0c
Compare
Description
This PR adds the Full Height Toolbar Alignment component to the block editor package, as well as its implementation to
Media & TextandCoverblocks.I did my first attempt to achieve this a long time ago in this PR. Although I couldn't finish it, it was very good to get feedback. I collected all ideas and implemented a solution (mostly a suggestion actually) to try to tackle all requirements. Spoiler Alter: Alignments never was for beginners.
Fixes #16385
Previous PRs #16738 #19767
Approach
Unlike the block alignment options,
Full Height Alignmentcan be thought of as an independent alignment type. It can be applied independently of other alignments, but also, it works very very well together with these block alignment options, where the combination empowers the design-layout capability.The buttons disposition in the toolbar could be something like the following:
Above, it shows the toolbar alignment options next to the full height alignment option.
Note: I did the icon, 100% opened to change itIntegrating with Cover block
The Cover block is a lovely case to see in action because this block also has implemented the min-height control. So the goal here is integrating this control with the Full Height Alignment. I'd like to say it was my idea, but it wasn't. Thanks, @mtias
In the following example, the block has
full width,full height, content positionleft bottomandfixed background.The result is very nice:
Full Height Alignment and Dimensions control.
I've connected both components to be consistent in the way that it applies the height to the cover block. In this case, Full Height Aligents acts just as a shorthand to apply
heightto100vh. You can at any time change or those values, and the button gets "untoggled" once that happens. In fact, It doesn't add any new attribute to the block.How has this been tested?
Screenshots
Types of changes
Checklist: