-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Try: Block Library polish #19836
Try: Block Library polish #19836
Conversation
Alright, I have updated this PR to make it work across breakpoints. As a first step on the journey towards #17335 (comment), this PR is now ready for review. To complete that task fully, we still need a separate tab for block patterns, and we need for the library to be possible to open as a modal from the sibling inserter. But as a first step, this PR can now be evaluated on its merits. Your thoughts are most welcome. I have updated the initial PR description with fresh GIFs and screenshots. |
Excellent feedback, Zeb, thank you for the review. Will take a stab at those now. |
Thank you, I believe I fixed the items mentioned, and an edgecase for non-fullscreen that I had forgotten about. |
Thanks. I'm under the weather today, so I'll take it easy and have this fixed tomorrow. |
a8fb641
to
b67a52b
Compare
Thanks to excellent testing, I believe this PR is ready to merge. Do note, however that as I worked to address #19836 (comment), I discovered an issue that also appears in master, which I've tracked in #19977. |
37b72ff
to
4a371c9
Compare
I'm have discovered and am working on an issue with this iteration which does not work too well when opened from the sibling inserter. |
4a371c9
to
9075ccb
Compare
Fixed an issue where these elaborate layout styles broke the minimalism of the appender library. As noted, that appender library is likely going to receive more love exploring next steps of #17335 (comment). But for now it's back to its old normal. |
contentClassName="block-editor-inserter__popover" | ||
contentClassName={ classnames( | ||
'block-editor-inserter__popover', | ||
{ 'is-from-toolbar': !isAppender } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open to suggestions for a better classname here than is-from-toolbar
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is-top-toolbar-inserter
, maybe?
f7e9ec1
to
f51dc36
Compare
This worked for the briefest of moments in a broken way in this branch, and as I debugged it I was confused too. Turns out its forcefully hidden in master also, and only visible when opened from the top left. I think unhiding it from that location is a worthwhile discussion, and one that is serendipitous to the next step ideas of making it a dialog when opened from those places. But I would think it may be best to keep the current behavior for this PR? |
If that's the behavior in |
This starts work on #17335 (comment). For starters, it redesigns the block library: - It is full-height to give lots more space. - It drops the help panel, as we now have a welcome screen. - It moves the tip to the footer. - It detaches the preview and shows it only on hover. Still to do here, is to polish the responsive behavior, possibly apply the new category names, and massage the pixels a bit further. What should also be done is to explore the UI from #18667, as well as add a tab for block patterns, and finally the ability for the block library to open as a modal dialog defaulting to the block patterns tab when opened from the sibling inserter. However these are probably best explored separately.
f51dc36
to
956cb60
Compare
contentClassName="block-editor-inserter__popover" | ||
contentClassName={ classnames( | ||
'block-editor-inserter__popover', | ||
{ 'is-top-toolbar-inserter': ! isAppender } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable used to distinguish the visual styling between the top toolbar inserter and the one invoked from the sibling inserter. In a rebase, I failed to verify this still worked before pushing, and somehow it broke. I don't know why it doesn't work anymore. @youknowriad if you have time, can you provide insights into this?
Here's how it looks when broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely not the right variable to check for and in fact we don't have any variable to distinguish these properly at the moment.
I think we follow the CSS suggestions I propose (avoid the large width and use absolute positionning for the preview), we could show the same inserter in all places and it will be shown properly with less code. We might need to add a dedicated prop to remove the preview hideBlockPreview
or somthing but the overall design will stay consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify the issue on that screenshot is that the width of the popover is very large (not just the panel on the left) so centering the toggle appear broken. Absolute positioning of the preview fixes that.
@include break-medium { | ||
overflow-y: visible; | ||
height: $block-inserter-content-height + $block-inserter-tabs-height + $block-inserter-search-height; | ||
.block-editor-inserter__toggle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this not top level?
@include break-medium { | ||
overflow-y: visible; | ||
height: 92vh; // This is limited by a max-height from the popover component itself. | ||
width: 82vw; // This is limited by a max-width. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like that this is using random numbers, this ignores the fact that the inserter is a reusable component that can be used anywhere.
Can we just remove these, have a max-width and height 100% (popover should adapt it according to the available space)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good thought, and is related to both #19836 (comment) and #19836 (comment).
The challenge is, we need for this to be responsive, i.e. fill the full height of the screen, and at least a big chunk of the screen horizontally at many breakpoints.
Absolute positioning can be used for that normally. But in this case, the popover container acts as a relative positioner, meaning that the coordinates, the width and the height of any child element is relative to that container. And it's 0 pixels wide and tall, which in this case was obviously solved with viewport units.
I'm afraid a refactor of this is probably beyond my skill to address.
pointer-events: none; | ||
} | ||
|
||
// Remove the popover styling and apply it instead to each sub panel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option is to consider the preview as an absolute positioned div outside of the flow of the popover. It might be simpler.
@jasmussen Tried a simpler approach here #20526 let me know what you think? |
Closing in favor of #20526, which is better in most ways! |
This starts work on #17335 (comment).
Desktop:
Medium sized desktop:
Smaller screens and mobile:
For starters, it redesigns the block library:
Still to do here, is to polish the responsive behavior, possibly apply the new category names, and massage the pixels a bit further.What should also be done is to explore the UI from #18667, as well as add a tab for block patterns, and finally the ability for the block library to open as a modal dialog defaulting to the block patterns tab when opened from the sibling inserter. However these are probably best explored separately.
For now and serving as a prototype,I appreciate how much space there is, including whitespace. The detached preview also feels lighter, even if it actually covers a bit more content. The amount of red in the code is also encouraging.