Make WordPress Core

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#39516 closed defect (bug) (fixed)

Media grid upload errors display below the grid.

Reported by: joemcgill's profile joemcgill Owned by: afercia's profile afercia
Milestone: 4.7.3 Priority: normal
Severity: normal Version: 4.6
Component: Media Keywords: has-patch fixed-major
Focuses: ui, accessibility, javascript Cc:

Description

In the media grid, whenever an error occurs we display that error in a media sidebar.

https://cldup.com/EGD14_07QK-3000x3000.png

Since [37610] (see #36909) the display order has changed so that the side bar is moved after the uploader and attachments list, which puts these errors after the grid, where they're not visible.

Attachments (7)

media-grid-errors.39516.diff (204.7 KB) - added by codegeass 8 years ago.
re-position the media grid sidebar
media-grid-errors.39516.2.diff (204.7 KB) - added by codegeass 8 years ago.
39516.diff (2.0 KB) - added by MatheusGimenez 8 years ago.
I've fixed the bug using conditionals according to the model.id of the object, if it's library, load the sidebar first, if not, load the uploader and attachments first.
uploader.mp4 (1.0 MB) - added by MatheusGimenez 8 years ago.
Working example video
39516.2.diff (2.0 KB) - added by MatheusGimenez 8 years ago.
Change code standards
39516.3.diff (1.8 KB) - added by afercia 8 years ago.
39516.4.diff (1.8 KB) - added by joemcgill 8 years ago.

Download all attachments as: .zip

Change History (42)

#1 @joemcgill
8 years ago

  • Component changed from General to Media

@codegeass
8 years ago

re-position the media grid sidebar

#2 follow-up: @SergeyBiryukov
8 years ago

Hi @codegeass, thanks for the patch!

There's no need to patch the minified or RTL files, those are generated automatically by the build script.

Also a note, attachments don't trigger notifications, so it's helpful to add a comment after uploading a patch.

#3 in reply to: ↑ 2 @codegeass
8 years ago

Hi @SergeyBiryukov;

So thanks, I was searching how to edit the minified files :)

Okey i will care about that.

Best,

Last edited 8 years ago by codegeass (previous) (diff)

#4 @joemcgill
8 years ago

#39569 was marked as a duplicate.

#5 @afercia
8 years ago

My bad 😬 There are good accessibility reasons to keep the current order in the source, see #36909. I'd recommend to don't revert the change. Looking for the simplest way to solve this bug, there's probably the need to distinguish two cases:

  • in the media modal: the sidebar is still displayed on the right (even if it's after the attachments in the source order) so, visually, everything works as expected (see screenshot below)
  • in the media Library though, the "sidebar" is displayed at the end of the attachments

Error messages are handled by wp.media.view.UploaderStatus and wp.media.view.UploaderStatusError and displayed inside the "sidebar". I guess trying to change the way the UploaderStatus is tied to the sidebar wouldn't be so easy.

As far as I see, in the Library view the "sidebar" is only used to display error messages so maybe one option could be to just create and place the sidebar conditionally:

  • if it's the Library view: create the sidebar before the attachments
  • otherwise: create the sidebar after the attachments

Any thoughts welcome.

Screenshot: error messages in the media modal sidebar:

https://cldup.com/xc4JU-50QX.png

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


8 years ago

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


8 years ago

#8 @afercia
8 years ago

  • Milestone changed from Awaiting Review to 4.7.2
  • Owner set to afercia
  • Status changed from new to assigned

Since this is a regression, I guess it should be fixed as soon as possible. Proposing for the next minor release.

@MatheusGimenez
8 years ago

I've fixed the bug using conditionals according to the model.id of the object, if it's library, load the sidebar first, if not, load the uploader and attachments first.

@MatheusGimenez
8 years ago

Working example video

#9 @MatheusGimenez
8 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


8 years ago

#11 follow-up: @afercia
8 years ago

Uploading a few screenshots made without the patch applied. They're meant as reference to check where the error messages should appear in different context. Some testing would be nice, the patch should not change any visual part of the UI, just the placement of elements in the source.

https://cldup.com/Qj-4mCHeHk.png

https://cldup.com/Yrtqc0_6Ak.png

https://cldup.com/OZ0ziIcW26.png

#12 follow-up: @adamsilverstein
8 years ago

@MatheusGimenez Thanks for the patch, it looks good!

A couple of code comments: for the model id check you should use a strict equality check and also switch the order to use Yoda conditions.

#13 in reply to: ↑ 11 @adamsilverstein
8 years ago

Replying to afercia:

Uploading a few screenshots made without the patch applied.

Thanks for the screenshots @afercia that's really helpful.

@MatheusGimenez
8 years ago

Change code standards

#14 in reply to: ↑ 12 @MatheusGimenez
8 years ago

Replying to adamsilverstein:

@MatheusGimenez Thanks for the patch, it looks good!

A couple of code comments: for the model id check you should use a strict equality check and also switch the order to use Yoda conditions.

Hi, i applied the code standards in the last patch.

Thanks for the feedback! :)

@afercia
8 years ago

#15 @afercia
8 years ago

I'd propose a slightly different approach, see refreshed patch.

Looking at the doc description for [options.sidebar=true], it Accepts true, false, and 'errors'. By default it's true and, as far as I see, it is set to 'errors' just once, for the media grid.

In the previous approach instead, the model id can have several values. Basically checking for the 'options.sidebar' value should be more reliable and the same check is already used a few lines below in browser.js.

Also, I'd propose to avoid nested ifs.

Worth mentioning the code could be simplified a bit, for example the multiple checks for 'errors' === this.options.sidebar could be avoided maybe, and the CSS classes could be set in just one go. But I didn't want to touch anything else ;)

#16 @afercia
8 years ago

Worth noting this could change again if the change proposed in #37188 will be approved.

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


8 years ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


8 years ago

This ticket was mentioned in Slack in #core-media by mike. View the logs.


8 years ago

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


8 years ago

@joemcgill
8 years ago

#21 @joemcgill
8 years ago

@afercia The approach in 39516.3.diff looks good, but let's use the state of the view instead of the value of the sidebar property, which is more likely to change. 39516.4.diff is exactly the same but checks for:

'library' === this.options.state

#22 @afercia
8 years ago

  • Keywords needs-refresh added

@joemcgill OK thanks :) I've used the sidebar property value just because it is already used a few lines below, for basically the same purpose (see below) so it seemed consistent to me. Either ways it's fine! I think 39516.4.diff missed to build media-views.js though.

https://cldup.com/6EP_Xc2QiA.png

#23 @joemcgill
8 years ago

  • Keywords commit added; needs-testing needs-refresh removed

@afercia good catch on the missed build.

I missed the use of this.options.sidebar earlier, and on second thought we should probably check that this.options.sidebar is not falsey before trying to render the sidebar anyway, so 39516.3.diff is good.

#24 @afercia
8 years ago

Thanks @joemcgill.
About the error when uploading a new image from the uploader:

https://cldup.com/Yrtqc0_6Ak.png

I see something changed from 4.5 to 4.6. In 4.5, when there is an error, the uploader view automatically switches to the grid view and the error gets displayed in the right column. Couldn't find the related change though. Any thoughts?

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


8 years ago

#26 @afercia
8 years ago

To recap: 39516.3.diff restores the errors displaying to what they looked like in 4.6. Wasn't able to track down the previous change from 4.5 to 4.6 though.

This ticket was mentioned in Slack in #core by jnylen. View the logs.


8 years ago

#28 @jnylen0
8 years ago

Per Slack discussion above, it looks like this is ready to go for 4.7.3, but the issue in comment:24 needs a separate ticket.

#29 @afercia
8 years ago

I think the issue in comment:24 is not because of changes in the media views JS, but it's because of [37727]. The related ticket #14244 omitted to highlight the (unintended?) change in the UI behaviour. Not a big deal I guess, just a bit difficult to trace down.

#30 @afercia
8 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 40126:

Media: Restore correct upload errors displaying after [37610].

Props codegeass, MatheusGimenez, joemcgill.
Fixes #39516.

#31 @afercia
8 years ago

  • Keywords fixed-major added; commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 4.7.3 consideration.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


8 years ago

#33 @joemcgill
8 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 40132:

Media: Restore correct upload errors displaying after [37610].

Props codegeass, MatheusGimenez, joemcgill.
Merges [40126] to the 4.7 branch.
Fixes #39516.

#34 @SergeyBiryukov
8 years ago

#39979 was marked as a duplicate.

#35 @joemcgill
8 years ago

#39989 was marked as a duplicate.

Note: See TracTickets for help on using tickets.