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.

Sign up for GitHub

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

Jump to bottom

Update for MediaPlaceholder component: don't stack error messages when re-uploading #14721

Merged
merged 10 commits into from
Jun 10, 2019
Merged

Update for MediaPlaceholder component: don't stack error messages when re-uploading #14721

merged 10 commits into from
Jun 10, 2019

Conversation

nicolad
Copy link
Member

@nicolad nicolad commented Mar 30, 2019

Description

Fixes: #6106
The implementation is based on this stale PR's feedback: #10250

How has this been tested?

It was tested locally.

Screenshots

Peek 2019-03-30 17-28

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@nicolad
Copy link
Member Author

nicolad commented Mar 30, 2019

Hello @noisysocks, taken into account your feedback from here: #10250
I've tried to fix the issue in 2 blocks.
Please confirm if that's ok and then I will add that fix in other blocks which use MediaPlaceholder.

@nicolad nicolad mentioned this pull request Mar 30, 2019
Closed
4 tasks
Soean
Soean previously requested changes Mar 30, 2019
View reviewed changes
Copy link
Member

@Soean Soean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, it works great in the audio block. We should do the same in the other blocks with upload option: Gallery, Media & Text, Video, Cover, File.

nicolad
nicolad commented Mar 30, 2019
View reviewed changes
packages/block-library/src/media-text/media-container.js
@@ -81,6 +92,8 @@ class MediaContainer extends Component {
onSelect={ onSelectMedia }
accept="image/*,video/*"
allowedTypes={ ALLOWED_MEDIA_TYPES }
notices={ noticeUI }
Copy link
Member Author

@nicolad nicolad Mar 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

noticeUI is not fitting properly here, should be addressed in a separate PR:
image

@nicolad
Copy link
Member Author

nicolad commented Mar 30, 2019

Thank you @Soean for taking time and reviewing this.
I have adjusted and tested locally all components you've mentioned.

@nicolad
Copy link
Member Author

nicolad commented Apr 20, 2019

@gziolo, may I also have your review on this?

@gziolo gziolo requested a review from aduth April 23, 2019 12:12
@gziolo gziolo added [Package] Notices /packages/notices [Feature] Media Anything that impacts the experience of managing media General Interface Parts of the UI which don't fall neatly under other labels. [Type] Enhancement A suggestion for improvement. and removed [Package] Notices /packages/notices labels Apr 23, 2019
aduth
aduth reviewed May 28, 2019
View reviewed changes
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patience in the review here. While there's separate ongoing discussion about how withNotices should be implemented or if it should be used (e.g. #9442), this seems like a sensible improvement on the current iteration of notices use in these components.

packages/block-library/src/audio/edit.js Outdated
const { noticeOperations } = this.props;
noticeOperations.removeAllNotices();
noticeOperations.createErrorNotice( message );
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spaces here ought to be a tab.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. thx.

@nicolad
Copy link
Member Author

nicolad commented May 29, 2019

Thanks for your patience in the review here. While there's separate ongoing discussion about how withNotices should be implemented or if it should be used (e.g. #9442), this seems like a sensible improvement on the current iteration of notices use in these components.

Thanks for letting me know this, I've subscribed to that discussion and related PRs in order to be on the same page and to apply the final solution when it will be available.

aduth
aduth approved these changes Jun 10, 2019
View reviewed changes
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work here. Tested across all of the impacted blocks and appears to work well 👍

@aduth aduth dismissed Soean’s stale review June 10, 2019 14:53

Feedback addressed (applied to requested blocks)

@aduth aduth merged commit f46d6f4 into WordPress:master Jun 10, 2019
@gziolo gziolo added this to the Gutenberg 6.0 milestone Jun 11, 2019
@nicolad nicolad deleted the mediaplaceholder-uploading-messages branch June 30, 2019 20:07
@nicolad nicolad self-assigned this Sep 29, 2019
This was referenced Jul 16, 2024
Open
Open
@OKEAMAH OKEAMAH mentioned this pull request Jul 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduth aduth aduth approved these changes

@Soean Soean Soean left review comments

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@gziolo gziolo Awaiting requested review from gziolo

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

@noisysocks noisysocks Awaiting requested review from noisysocks

@notnownikki notnownikki Awaiting requested review from notnownikki

@talldan talldan Awaiting requested review from talldan

@youknowriad youknowriad Awaiting requested review from youknowriad

Assignees

@nicolad nicolad

Labels
[Feature] Media Anything that impacts the experience of managing media General Interface Parts of the UI which don't fall neatly under other labels. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Milestone
Gutenberg 6.0
Development

Successfully merging this pull request may close these issues.

Uploading messages for media shouldn't stack
4 participants
@nicolad @Soean @aduth @gziolo