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

GalleryBlock: place edit button in separate toolbar. #1136

Merged
merged 2 commits into from
Jun 12, 2017

Conversation

mtias
Copy link
Member

@mtias mtias commented Jun 11, 2017

image

Fixes #1129.

@jasmussen do you want to add the proper icon?

@mtias mtias added the [Feature] Blocks Overall functionality of blocks label Jun 11, 2017
@mtias mtias requested a review from jasmussen June 11, 2017 22:30
@jasmussen
Copy link
Contributor

Nice!

Like @njpanderson, we should use the edit icon here. That's what the current gallery uses also.

@mtias mtias force-pushed the update/place-edit-gallery-toolbar branch from 54f87c9 to 87bcdc9 Compare June 12, 2017 07:59
@jasmussen
Copy link
Contributor

I dig it! Works well. 👍

@mtias mtias merged commit ffeadcc into master Jun 12, 2017
@mtias mtias deleted the update/place-edit-gallery-toolbar branch June 12, 2017 12:26
@aduth
Copy link
Member

aduth commented Jun 12, 2017

Two things:

  • We created BlockControls (and later similarly InspectorControls) as abstractions to obscure the underlying fill behavior and naming. We should decide whether this is a pattern we want to embrace and apply it consistently.
  • On the point of consistency, while perhaps out of scope of this pull request, seems we should move all controls to be rendered as descendants of the edit component.

@youknowriad
Copy link
Contributor

On the point of consistency, while perhaps out of scope of this pull request, seems we should move all controls to be rendered as descendants of the edit component.

Agree, That was my intention after merging #1019

@paaljoachim
Copy link
Contributor

paaljoachim commented Jun 12, 2017

This is really picky but I feel there is too much space between the icons in the toolbar:
toolbar

Here is one that shows the two groups closer together.

toolbar

I believe all toolbars that have groupings should have a less space between the groups.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants