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

Block gallery: add support for image reordering #14768

Merged
merged 14 commits into from
May 20, 2019
Merged

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Apr 2, 2019

This PR implements the basic mechanics to move the selected image forward/backward within a gallery. Also adds the corresponding visual controls.

Peek 2019-05-10 14-46

@oandregal oandregal self-assigned this Apr 2, 2019
@oandregal oandregal added [Type] Enhancement A suggestion for improvement. [Block] Gallery Affects the Gallery Block - used to display groups of images labels Apr 2, 2019
@oandregal
Copy link
Member Author

cc @jasmussen

@ellatrix
Copy link
Member

ellatrix commented Apr 2, 2019

Why limit it to the plugin only?

@oandregal
Copy link
Member Author

Why limit it to the plugin only?

Developing this only for the plugin provides a safe space for experimentation. This is why I like that space: I consider this PR the first step in adding DragNDrop support for galleries, and there are a couple of different options we can use as the drag handle for the selected image. These controls may be affected by that later decission, so I wouldn't want to introduce them to only change them in a later iteration, if needed.

@ellatrix
Copy link
Member

ellatrix commented Apr 2, 2019

Cool, was asking because these controls already seem quite valuable to users.

@jasmussen
Copy link
Contributor

Really nice work. Perhaps you're already on this, but I'm not seeing the controls show up in the forwards tabbing order, and I'm seeing some weird tabbing behavior that may be already present in master. GIF:

tabbing

First issue: when I tab, I first select the image, then the caption, then the next image. But the previous image still stays in what appears to be a focused state. When I tab on to the next thumbnail, the previous thumbnail should no longer be focused.

The 2nd issue is, I would expect the tabbing order to be:

  1. Thumbnail
  2. Move left if not disabled
  3. Move right if not disabled
  4. Remove image
  5. Caption
  6. Next thumbnail, etc.

Is this within workable?

@oandregal
Copy link
Member Author

oandregal commented Apr 4, 2019

First issue: when I tab, I first select the image, then the caption, then the next image. But the previous image still stays in what appears to be a focused state. When I tab on to the next thumbnail, the previous thumbnail should no longer be focused.

This is also an issue on master that I've filled at #14800 and prepared a fix for at #14813

The 2nd issue is, I would expect the tabbing order to be (controls and the caption).

This is also an issue on master: note how the delete control is only focused when you tab backward.

I'd like to tackle this separately and receive some a11y feedback. My first thought when I saw it was: "well, this is not what I expected but perhaps is a good thing because it shortens the keyboard jumps I have to do for the common actions (move through images/edit captions/etc.)".

I'm also open to fix this here if you feel like adding the moving controls totally change the expectations of what the common actions are, though.

@jasmussen
Copy link
Contributor

Okay to tackle separately for sure, but definitely something we'll want someone to tackle at some point.

"well, this is not what I expected but perhaps is a good thing because it shortens the keyboard jumps I have to do for the common actions (move through images/edit captions/etc.)".

Yes, but unfortunately it does so at the cost of inconsistency and unpredictability.

@oandregal
Copy link
Member Author

Okay to tackle separately for sure, but definitely something we'll want someone to tackle at some point.

I meant that I will fix this as part of my work with the gallery! Tracked at #14814

@oandregal oandregal force-pushed the add/gallery-move-images branch from ed24ea2 to bfcc397 Compare April 4, 2019 08:46
@oandregal
Copy link
Member Author

Note that the fix for two images being selected has already landed in master so I've rebased this branch to include it here.

@oandregal oandregal added this to the 5.5 (Gutenberg) milestone Apr 9, 2019
@oandregal oandregal requested a review from youknowriad April 11, 2019 09:03
@youknowriad
Copy link
Contributor

Let's try to find a way forward here, can we always show the controls but "visibly hidden" if the block is not selected, that way the tab order is always consistent? Also it seems that when you tab to the image, the container should be focused first.

@oandregal
Copy link
Member Author

Second attempt to fix the gallery tab order can be reviewed at #15540 The idea would be to merge that and then rebase this PR on top.

@oandregal oandregal force-pushed the add/gallery-move-images branch from 63e0bf0 to a3e5854 Compare May 10, 2019 12:17
@oandregal
Copy link
Member Author

oandregal commented May 10, 2019

The fix for the tab order has landed and this has been rebased. It's ready for another review round!

cc @youknowriad @jasmussen

@oandregal oandregal requested a review from youknowriad May 10, 2019 12:48
@youknowriad
Copy link
Contributor

Is it just me or the arrow buttons are not working anymore?

@youknowriad
Copy link
Contributor

This is working great for me, I'll leave the final word for the others.

@oandregal
Copy link
Member Author

@jasmussen would you try this and 👍 if appropriate?

@youknowriad youknowriad requested a review from jasmussen May 16, 2019 20:53
Copy link
Contributor

@jasmussen jasmussen 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 a remarkable improvement. Really well done. It is foundational accessibility built well:

gallery

@oandregal oandregal merged commit 5169edb into master May 20, 2019
@oandregal oandregal deleted the add/gallery-move-images branch May 20, 2019 07:32
@youknowriad youknowriad added this to the 5.8 (Gutenberg) milestone May 24, 2019
@youknowriad youknowriad mentioned this pull request Aug 19, 2019
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Gallery Affects the Gallery Block - used to display groups of images [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Design Feedback Needs general design feedback. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants