-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Media: Refactor the MediaUploadButton to be agnostic to its rendered UI #4513
Conversation
> | ||
{ __( 'Add from Media Library' ) } | ||
</MediaUploadButton> | ||
render={ ( { open } ) => ( |
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.
"Children as function" would also work but I personally prefer "render props". I prefer if children would have been always used as "arrays" to avoid confusion in the shape of Children in general.
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 heard in Egghead.io podcast that render props
is easier to follow for the majority of developers. I prefer it over this awkward this.props.children()
, too.
17b42d1
to
410a06e
Compare
const { children, buttonProps, tooltip } = this.props; | ||
|
||
return ( | ||
<MediaUpload |
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.
Ha, nice! :)
blocks/media-upload/README.md
Outdated
MediaUpload | ||
=========== | ||
|
||
Dropdown is a React component used to render a button that opens a the WordPress media modal. |
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.
Copy and paste here :)
Love this refactoring. In general, I'm always happy seeing when I will do some tests before accepting this PR. |
410a06e
to
1ba726b
Compare
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.
Tests also well 👍
We're using the
MediaUploadButton
in several places and the button triggering the modal is very different from a place to another:To address all the use-cases we were slowly adding complexity to the render method of the
MediaUploadButton
. This PR solves this issue by relying on a render prop instead. That way we can reuse any other component we use regularly to render these buttons.This PR also adds a README to document this component and deprecates the old component.