-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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
Replace placeholder handling for MediaPlaceholder instructions #10861
Conversation
instructions = __( 'Drag an audio, upload a new one or select a file from your library.' ); | ||
} else if ( isEmpty( difference( allowedTypes, [ 'image', 'video' ] ) ) ) { | ||
instructions = __( 'Drag an image or a video, upload a new one or select a file from your library.' ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding all these if/else here, can we use just a generic string here and pass these as props where appropriate? that way we stay consistent in how we deal with labels in this component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, had the same thought. One point for doing it like this is having the translations provided by core. So third-party usage of MediaPlaceholder
don't have to provide their own translations for the same strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be ok with it if the "title" is handled similarily...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also wonder if we should have a generic "Media" in the text if the allowedTypes
contains more than one item.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is already the case in https://github.com/WordPress/gutenberg/pull/10861/files#diff-56d6af193779467f7a4cb10e85c26ef3R170, unless I'm missing something.
instructions = __( 'Drag an image, upload a new one or select a file from your library.' ); | ||
} else if ( isAudio ) { | ||
instructions = __( 'Drag an audio, upload a new one or select a file from your library.' ); | ||
} else if ( isImageAndAudio ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we should leave this one as a prop (image and video) as it seems too specific to be bundled in the component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I already left out the title definition for this case.
|
||
### Breaking Changes | ||
|
||
- The `labels.name` property has been removed from `MediaPlaceholder` in favor of the new `labels.instructions` prop. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So yes, this is a breaking change but I don't expect it to have big negative impacts on people already using the component, so I think I'm fine shipping without deprecation handling. Thoughts @ocean90 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, the deprecation handling is actually the new handling for default title/instructions. No JS errors or similar will be produced by this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates. LGTM 👍
…ress#10861) Add default title and instructions labels to MediaPlaceholder based on allowed types.
Description
Fixes #10855.
labels.name
instructions
string based on allowed typeslabels.instructions
How has this been tested?
Add blocks with media uploads and verify that all instructions still make sense. Run
npm run test
to check unit tests.