Media Utils: Don't convert error messages into an array #39448
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: -57 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
Mamaduka
added
[Feature] Media
|
I wonder why was it sent as array in that specific format in the 1st place? Was it some sort of core compatibility? |
There was a problem hiding this comment.
Thanks for the ping and for looking into this @Mamaduka!
In this case, it looks like this change removes presenting the filename to the user in the error state. I think I prefer displaying it at this stage, so that it's clear which file there was a problem with. If we change the shape of what we're calling onError with, should we then update each of the onUploadError handlers to include the filename?
| Before | After |
|---|---|
 |
 |
I think I see why the array approach might have been used, so that each of the notices don't have to add in the <strong>{ error.file.name }</strong> on their own? Perhaps we could inline that in each of the blocks' edit functions, otherwise a tiny component could be used to abstract that away. It's such a tiny bit of logic though, that it's sort of tricky to know where to best put it!
|
That was a long time ago. 🙂 I do recall working on this PR, but don't remember the details. As I noted in the comment with it, the The absence of the filename is particularly noticeable if you try to upload several files, some of which cause errors, and some of which don't. It's very confusing to figure out which file caused which message. |
|
Thanks for the feedback, everyone. I agree that the filename is helpful in the message, so maybe it should become part of the message? HTML isn't supported in some types of notices, like Snackbars. |
Yes, I remember my surprise with HTML in errors while working on the media replace flow. If the only reason is to let the user know about the filename, then it should be as part of the message, as a string. @Mamaduka should this change be deprecation? I saw you searched for eventual problems but ... who can tell :) |
Personally, I don't mind if it's concatenated as part of the message, or if the code calling |
|
@draganescu I don't think we can use deprecation here unless we want to deprecate the I think filename being part of the message also could be helpful for translators. Plus major release is the best time to introduce string change 😄 |
draganescu
approved these changes
May 16, 2022
+1 that sounds good to me, it looks like this PR just needs a rebase and to restore concatenating the filename into the error message string (which we could always do separately if need be, too) |
|
Thanks for the reviews, @andrewserong and @draganescu. I will rebase the branch and update the messages to include file names. |
Mamaduka
force-pushed
the
update/upload-media-message-type
branch
from
560ee29 to
32322eb
Compare
|
Updated message string to include file names. I've not changed the messages themselves; they would match the previous structure. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
It makes the message type passed to the
onErrorcallback consistent.Why?
Based on usage in the core, messages are cast back to the strings, or the actual message part is extracted from the array. Returning single type should improve the developer experience.
How?
I've removed the
triggerErrorhelper, and now messages errors are directly passed into theonErrorcallback.I searched WP Directory/GitHub. Based on the results, this change shouldn't cause any breakage.
Testing Instructions
Tests should pass.