MediaPlaceholder: Fix media library button opening the file upload modal #34894
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
andrewserong
added
[Type] Bug
andrewserong
requested review from
glendaviesnz,
ellatrix,
jorgefilipecosta and
ntsekouras
Closed
2 tasks
|
I've added a few folks who've recently worked on the changed file as reviewers, but please let me know if there's anyone else I should ping instead 🙂 |
|
Size Change: -8 B (0%) Total Size: 1.06 MB
ℹ️ View Unchanged
|
glendaviesnz
approved these changes
Sep 17, 2021
There was a problem hiding this comment.
This tested well for me. I tested the Media placeholder with this patch in place against:
✅ Old gallery block
✅ New gallery block
✅ Individual image block
In each case clicking the Media Library button open the media browser as expected with no upload dialog loading by default.
The file dialog still loaded as expected using the Select files button in the Media library or the 'Upload' button on the Media placeholder.
ramonjd
approved these changes
Sep 20, 2021
There was a problem hiding this comment.
✅ From the placeholder state, click the Media library button — observe that the media library opens, but the file dialog does not
✅ Click elsewhere in the Placeholder — neither the media library nor the file dialog should open
✅ Click on the Upload button — the file dialog should open
I propose we remove the onClick handler on the overall Placeholder and move it down to the Upload button, to avoid unexpected behaviour.
I agree. Binding openFileDialog() to the element that "opens the file dialog" make prima facie sense, and I think will avoid side effects in the future.
🚢
andrewserong
deleted the
fix/media-placeholder-file-modal-opening-unexpectedly
branch
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.
Description
Fixes #34099
This PR fixes an issue with the appender state of the MediaPlaceholder component where clicking the "Media library" button causes the file dialog modal to open. A common place to test this is with adding an image to a gallery that already has images.
Background on the issue
The regression appears to have been introduced in #33632 which removed the
event.stopPropagation()call on themediaLibraryButtonso the click event bubbles up to the Placeholder component, which in theisAppenderstate opens up the file dialog.Proposed fix
While we could re-instate the
event.stopPropagation()call, I propose we remove theonClickhandler on the overall Placeholder and move it down to the Upload button, to avoid unexpected behaviour. This is just my personal preference of course, so happy for design feedback on this! CC: @jorgefilipecosta as it looks like the original behaviour was introduced back in #12367How has this been tested?
Manually in the editor. In looks like there were no other uses of the
onClickparam in therenderPlaceholderfunction, but do let me know if you can think of any uses I've missed!Prior to applying this PR
After applying this PR
Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).