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

Align Gallery block movers with the standard block mover style #17316

Merged
merged 5 commits into from Sep 6, 2019

Conversation

@kjellr
Copy link
Contributor

commented Sep 3, 2019

Followup from #17216.

This PR updates the Gallery block's individual item controls to align with the style used for block movers. As part of that, it switches from using Dashicons for the icons to Material SVGs to match.

Before

Screen Shot 2019-09-03 at 1 33 54 PM

After

Screen Shot 2019-09-03 at 1 33 22 PM

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

Nice, I like the improved consistency.

This might belong in a separate PR, but shouldn't the "X" button use a trash icon instead? "X"s are usually associated with closing, not deleting, and Gutenberg already uses a trash icon for removing blocks.

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

This might belong in a separate PR, but shouldn't the "X" button use a trash icon instead? "X"s are usually associated with closing, not deleting, and Gutenberg already uses a trash icon for removing blocks.

That's an interesting idea, thanks for bringing it up.

I would say no, though. The X, I would suggest, is for dismissing, not closing, whereas the trash can as you note is for deleting. In the context of the gallery you aren't actually deleting an image from your media library, you are dismissing it from the gallery. So I would say the X is still appropriate. But pretty good observation regardless.

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

@jasmussen If the images in galleries became nested blocks, wouldn't they then use a trash icon for removal? You're not deleting an image in that case either, but you are deleting an instance of the image. And I'd argue you're doing the same thing here: deleting an instance of an image.

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

Another solid observation. Depending on how the child block implementation for gallery looks, it might be more or less relevant. While the benefit of using child blocks is that we get things like the movers, drag and drop and other block properties "for free", to an end user I don't know that they care what those innerblocks are made of, so in a way the UI could and possibly should be the same. There are some further thoughts around those lines in #17267.

I'm not strongly attached to this opionion, though, it's mostly an instinct, and if there's agreement the trash icon is the way to go, then blaze a trail. Personally though, I still think logic favors the X.

@kjellr

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

I agree that when/if the Gallery block becomes full of nested blocks, a trash can might make more sense there. But in the near term, it makes sense to keep the x (at least for this specific PR).

@shaunandrews

This comment has been minimized.

Copy link

commented Sep 5, 2019

Very nice change.

I do wonder about the direction of the arrows — they are always horizontal, but due to the nature of the Gallery block they can sometimes move an image vertically.

F3FEF171-3444-4CE8-9F97-699D13D05E7F

As for the “x” icon, it’s worth mentioning that this closely matches the design for managing galleries in the Media Modal:

8E6D261A-984F-49E2-8FC6-A22F21A48A61

@kjellr

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

I do wonder about the direction of the arrows — they are always horizontal, but due to the nature of the Gallery block they can sometimes move an image vertically.

Yeah, that's definitely a little weird. I'm not sure there's much we can do about that just yet (without a ton of extra code at least), so it may need to wait until we make this a nested block situation, when these movers will be rewritten anyway.

In the meantime, I think this PR is good to go. Does someone mind giving it a final review and ?

@kjellr kjellr merged commit b9dc48c into master Sep 6, 2019

2 checks passed

pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details

@kjellr kjellr deleted the update/gallery-item-menus branch Sep 6, 2019

@kjellr kjellr added this to the Gutenberg 6.5 milestone Sep 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.