Skip to content
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

Closed
wants to merge 6 commits into from
Closed

Try: Block Library polish #19836

wants to merge 6 commits into from

Conversation

jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Jan 23, 2020

This starts work on #17335 (comment).

Desktop:

desktop

Medium sized desktop:

Screenshot 2020-01-24 at 13 09 39

Smaller screens and mobile:

Screenshot 2020-01-24 at 13 10 02

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.

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.

@jasmussen jasmussen added the [Status] In Progress Tracking issues with work in progress label Jan 23, 2020
@jasmussen jasmussen self-assigned this Jan 23, 2020
@jasmussen jasmussen added [Feature] Inserter The main way to insert blocks using the + button in the editing interface [Package] Block library /packages/block-library and removed [Status] In Progress Tracking issues with work in progress labels Jan 24, 2020
@jasmussen
Copy link
Contributor Author

jasmussen commented Jan 24, 2020

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.

@ZebulanStanphill
Copy link
Member

I like it!

I think Reusable Blocks belong in their own tab. They're sort of like patterns but with shared state across all instances, so I think it makes sense to separate them from standard blocks.

Some issues I noticed:

The preview for the Group block overflows vertically.
image

The block library is too thin at some viewport widths.
image

The help/tip text needs some more padding.

Also, the sibling inserter button is missing the white background color it had before, though I haven't checked to see if the same issue exists in master.
image

@jasmussen
Copy link
Contributor Author

Excellent feedback, Zeb, thank you for the review. Will take a stab at those now.

@jasmussen
Copy link
Contributor Author

Thank you, I believe I fixed the items mentioned, and an edgecase for non-fullscreen that I had forgotten about.

@ZebulanStanphill
Copy link
Member

Nice! Unfortunately I just discovered another issue. It looks like the inserter doesn't fill the screen width properly on mobile.

image

It looks like it may have something to do with scrollbars taking up space (or not) on different platforms.

@jasmussen
Copy link
Contributor Author

Thanks. I'm under the weather today, so I'll take it easy and have this fixed tomorrow.

@jasmussen
Copy link
Contributor Author

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.

@jasmussen
Copy link
Contributor Author

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.

@jasmussen
Copy link
Contributor Author

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 }
Copy link
Contributor Author

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.

Copy link
Member

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?

@ZebulanStanphill
Copy link
Member

I have noticed one more minor issue. The block previews do not appear for the inserter when opened from the placeholder (both the textual Paragraph variant and the rectangle one used in the Group and Column blocks).
image

@jasmussen
Copy link
Contributor Author

The block previews do not appear for the inserter when opened from the placeholder (both the textual Paragraph variant and the rectangle one used in the Group and Column blocks).

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?

@ZebulanStanphill
Copy link
Member

If that's the behavior in master, then I think it's fine to merge this without any further changes (after merge conflicts are resolved).

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.
contentClassName="block-editor-inserter__popover"
contentClassName={ classnames(
'block-editor-inserter__popover',
{ 'is-top-toolbar-inserter': ! isAppender }
Copy link
Contributor Author

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.

Screenshot 2020-02-03 at 10 08 37

Copy link
Contributor

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.

Copy link
Contributor

@youknowriad youknowriad Feb 11, 2020

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 {
Copy link
Contributor

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.
Copy link
Contributor

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)?

Copy link
Contributor Author

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.
Copy link
Contributor

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.

@youknowriad
Copy link
Contributor

@jasmussen Tried a simpler approach here #20526 let me know what you think?

@jasmussen
Copy link
Contributor Author

Closing in favor of #20526, which is better in most ways!

@jasmussen jasmussen closed this Mar 3, 2020
@youknowriad youknowriad deleted the try/block-library-polish branch March 3, 2020 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Inserter The main way to insert blocks using the + button in the editing interface [Package] Block library /packages/block-library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants