#39516 closed defect (bug) (fixed)
Media grid upload errors display below the grid.
Reported by: | joemcgill | Owned by: | afercia |
---|---|---|---|
Milestone: | 4.7.3 | Priority: | normal |
Severity: | normal | Version: | 4.6 |
Component: | Media | Keywords: | has-patch fixed-major |
Focuses: | ui, accessibility, javascript | Cc: |
Attachments (7)
Change History (42)
#2
follow-up:
↓ 3
@
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
@
8 years ago
Hi @SergeyBiryukov;
So thanks, I was searching how to edit the minified files :)
Okey i will care about that.
Best,
#5
@
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:
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
@
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.
@
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.
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
8 years ago
#12
follow-up:
↓ 14
@
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
@
8 years ago
Replying to afercia:
Uploading a few screenshots made without the patch applied.
Thanks for the screenshots @afercia that's really helpful.
#14
in reply to:
↑ 12
@
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! :)
#15
@
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 if
s.
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
@
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
#21
@
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
@
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.
#23
@
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
@
8 years ago
Thanks @joemcgill.
About the error when uploading a new image from the uploader:
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
@
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
@
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
@
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.
#31
@
8 years ago
- Keywords fixed-major added; commit removed
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening for 4.7.3 consideration.
re-position the media grid sidebar