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.

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

Cover image: added upload button. #4361

Merged
merged 4 commits into from Jan 11, 2018

Conversation

jorgefilipecosta
Copy link
Member

This PR adds an upload button to cover image to make it consistent with the image block.
Fixes: #4341

How Has This Been Tested?

Add a cover image, use the upload button select an image and verify the image was used in the cover image block.

Screenshots (jpeg or gifs if applicable):

screen shot 2018-01-08 at 23 01 15

@jorgefilipecosta jorgefilipecosta added [Type] Enhancement A suggestion for improvement. [Feature] Blocks Overall functionality of blocks labels Jan 8, 2018
@jorgefilipecosta jorgefilipecosta self-assigned this Jan 8, 2018
@mtias
Copy link
Member

mtias commented Jan 9, 2018

It would be great if all these became an ImagePlaceholder component that came with the upload buttons, drag area, etc. The text and title should probably be customizable.

*/
import MediaUploadButton from '../media-upload-button';

export default function ImagePlaceHolder( { className, setAttributes, icon, label, onSelectImage } ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Great work extracting this, there's something that bother me a bit about the setAttributes being passed here and also being passed to mediaUpload (probably the root issue).

Instead of passing setAttributes to mediaUpload, should we refactor it to pass something like onChange and onChange would be called with a single or multiple images (it's used in gallery aswell).

This would allow us to only pass onChange here.

The issue with the current approach is that this "reusable" component enforces the block using it to have certain attribute keys.

Copy link
Member Author

Choose a reason for hiding this comment

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

I totally agree with this changes, the refactor should be applied in the media upload because it is there that we are using the attribute keys. I will add to this PR a refactor to the media upload as previous work.

Copy link
Member Author

Choose a reason for hiding this comment

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

A refactor was applied to the mediaUpload. It is now more generic and does not contain block specific logic e.g: for the gallery.
The placeholder now does not receive setAttributes it just receives onSelectImage that is used for both uploaded and gallery images. An object is passed to function describing the image, the user of the component can pass a function that maps the image object to its attributes.

@jorgefilipecosta jorgefilipecosta force-pushed the add/upload-button-cover-image branch 2 times, most recently from 82012dc to ccff0d4 Compare January 10, 2018 18:23
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

This needs a rebase, but awesome work on this PR

@jorgefilipecosta jorgefilipecosta force-pushed the add/upload-button-cover-image branch 5 times, most recently from 4507681 to 06984a4 Compare January 11, 2018 20:07
@jorgefilipecosta jorgefilipecosta force-pushed the add/upload-button-cover-image branch 2 times, most recently from a24de71 to 0fd78d4 Compare January 11, 2018 21:04
mediaUpload now receives receives an array of files and a function that it calls with an array of objects representing the files uploaded. Logic specific to gallery and image blocks was removed from the function.
@jorgefilipecosta jorgefilipecosta merged commit 23bf408 into master Jan 11, 2018
@jorgefilipecosta jorgefilipecosta deleted the add/upload-button-cover-image branch January 11, 2018 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants