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

Collapsible panels for the block's toolbar #9687

Merged
merged 10 commits into from
Sep 14, 2018
Merged

Conversation

youknowriad
Copy link
Contributor

closes #9075

This PR adds

  • Support for "collapsible" toolbars using the isCollapsed prop
  • Support for collapsing text alignment toolbars
  • Auto-collapse text alignment toolbars on mobile
  • Always Collapse text alignment for the paragraph block.

I tried adding auto computation on whether or not to collapse the toolbar segments based on the available space. It turns out it's more challenging than I expected originally and may not be achievable in a not fragile way, I'm instead suggesting it should be "opt-in" like the paragraph block here or more simple computations like "mobile" automatically triggering collapsed panels.

@youknowriad youknowriad added the Needs Design Feedback Needs general design feedback. label Sep 7, 2018
@youknowriad youknowriad self-assigned this Sep 7, 2018
@youknowriad youknowriad requested review from mtias, jasmussen and a team September 7, 2018 11:19
@jasmussen
Copy link
Contributor

This is a very nice first stab. GIF:

toolbar segments

I'd like to do some stule changes — tweak the dropdown arrow a bit, tweak the focus and active styles, that sort of thing. But this is working very well.

It is rather unfortunate that we can't auto-collapse when space is at a premium, and uncollapse when space is aplenty. Is there anything we can do to make this easier? Are there any changes in the design or structure that could accommodate this collapsing, even if those changes come with their own tradeoffs?

Did anything come of the element query discussions? I had my doubts about using polyfills here given how react creates the DOM, but hope springs eternal.

@youknowriad
Copy link
Contributor Author

Are there any changes in the design or structure that could accommodate this collapsing, even if those changes come with their own tradeoffs?

It's not about design structure but more about Block API and the way we declare the toolbars.

Did anything come of the element query discussions?

element query is about the size of the container not about the available space (like media queries) so not really, they can't help a lot

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good 👍 I only found a problem with headings on mobile because of the way the headings on mobile use the AlignmentToolbar.

return (
<Toolbar
isCollapsed={ isCollapsed || ! isLargeViewport }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we may need an option to disable this forced collapsing on mobile. On the heading block the AlignmentToolbar component is used on the sidebar and collapsing it makes the result feel strange.
sep-07-2018 19-13-29-1

If the resolution is really small it becomes even impossible to change the alignment, that seems unexpected and I'm not certain of the reason why it happens:
sep-07-2018 19-14-03-2

@jorgefilipecosta
Copy link
Member

It is rather unfortunate that we can't auto-collapse when space is at a premium, and uncollapse when space is aplenty. Is there anything we can do to make this easier?

Maybe we can get this result with some assumptions. Like on mobile we can consider space a premium like we do and always collapse. On the desktop for the paragraph case, if it is not nested, we can decide that we always have space and never collapse. If it is nested depends on the parent to determine if there is space or not and the parent may decide if the paragraph alignment options are collapsed or not. To pursue this option, we would need to implement some kind of options API that allows parents to control the UI of children as referred in #7931.
A more simple version would be for the alignment options of the paragraph to be collapsed when the paragraph is nested and uncollapsed when it is not nested (we can get if the paragraph is nested or not with a simple withSelect call). But even the simplest version adds some complexity to a block that is expected to be simple like the paragraph.

@jasmussen
Copy link
Contributor

I like this line of thinking, Jorgé. It brings us benefits immediately, despite the challenges.

Non-mobile, non-nested blocks that can fit the expanded segments should have them. Mobile, nested, or floated blocks, should have collapsed segments. Seems like a great compromise.

@youknowriad
Copy link
Contributor Author

I updated this PR and I'm now auto-collapsing the alignment toolbar on:

  • small screens
  • nested context when the toolbar is not fixed at the top

I didn't add the collapsing when the block is "floated" because the block being floated or not is a block attribute value (the attribute name can change) and can't be detected in a generic way.

@youknowriad youknowriad force-pushed the add/collapsible-segments branch from bec7890 to 1c954c4 Compare September 12, 2018 08:33
@jasmussen
Copy link
Contributor

Awesome work Riad, this is working really well. Mobile:

screen shot 2018-09-12 at 10 52 01

Nested:

screen shot 2018-09-12 at 10 51 24

Can we add this for Image/Gallery/Cover Image or in general decorative alignemnts too?

screen shot 2018-09-12 at 10 50 55

I love how the menu is fully tabbable as it should be.

Let me know when's a good time and I'll push some polish to the indicators — both the dropdown triangle and the focus styles could use a little love, and we need the "active" indicators to show in the dropdown as well — which I can probably fix.

@youknowriad
Copy link
Contributor Author

I added the "collapsible" behavior to the block alignment toolbar as well

@jasmussen
Copy link
Contributor

I'll try and take a look at the dropdown/focus styles in a minute.

@mtias mtias added [Type] Task Issues or PRs that have been broken down into an individual action to take and removed Needs Design Feedback Needs general design feedback. labels Sep 12, 2018
@mtias mtias added this to the 3.9 milestone Sep 12, 2018
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is looking good. I did some tests, and things seem to be working fine.
Maybe we can add some end two end tests for this behaviors so we can catch regressions easily (can be done in a separate PR).
As @jasmussen referred, it would be nice to have indications on the dropdown for the currently active item.
I would say that for example for the block alignment we should also add a gray mark to the icon (even before drop-down is open) if any alignment is enabled (identical to the marks we had before), so the users can differentiate between no alignment and left alignment even without opening the drop-down.
Right now it is impossible to differentiate both states (using the block toolbar):
image

@youknowriad
Copy link
Contributor Author

I added a background on the active item similar to how it shows in the toolbar.

@jasmussen
Copy link
Contributor

Thanks for that, Riad. I will be using that to polish it a bit further.

@jasmussen
Copy link
Contributor

Okay, I've pushed some polish. Sorry it took so long.

  • Dropdown arrow now the same as in switcher
  • Formatting menu items look and behave the same in the dropdown
  • Dropdown itself shows the active state, it doesn't show the "toggled" state.

new styles

@jasmussen
Copy link
Contributor

Given a sanity check of c933243 and 89790a6, I think this is good to go experience wise. 👍 👍 amazing work Riad!

}

&:not(:disabled):not([aria-disabled="true"]):not(.is-default).is-active {
@include formatting-button-style__active();
// Formatting buttons
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not only for formatting buttons though it's for any Dropdown menu containing icons right? (like the table block)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but the table block will never have an is-active applied, so the dark background won't be applied.

<IconButton
key={ [ indexOfSet, indexOfControl ].join() }
onClick={ ( event ) => {
if ( control.isDisabled ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should we expect onClick callback would ever be triggered when disabled prop is passed?

Speaking more from a high-level; this doesn't seem like it should happen.

Edit: Observing this is a hold-over from previous implementation. My concern still stands, but not necessarily within scope of the pull request.

const { getBlockRootClientId, getEditorSettings } = select( 'core/editor' );
return {
isCollapsed: isCollapsed || ! isLargeViewport || (
getBlockRootClientId( clientId ) &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: getBlockRootClientId's implementation is computationally intensive (aside: it should be improved), and thus we could reorder the && parts here to short-circuit on getEditorSettings as the fast path.

@youknowriad youknowriad merged commit 762c717 into master Sep 14, 2018
@youknowriad youknowriad deleted the add/collapsible-segments branch September 14, 2018 09:58
@mtias
Copy link
Member

mtias commented Sep 14, 2018

Nice to see this in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow toolbar segments to collapse (responsive toolbar)
5 participants