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

Filter DropZone and MediaPlaceholder components #11184

Merged
merged 8 commits into from
Oct 30, 2018
Merged

Filter DropZone and MediaPlaceholder components #11184

merged 8 commits into from
Oct 30, 2018

Conversation

mikeselander
Copy link
Contributor

@mikeselander mikeselander commented Oct 29, 2018

Description

Resolves #10177.

Export both the DropZone and MediaPlaceholder editor components with the withFilters HOC. This makes those components overridable on a more granular basis and makes the media components easier to modify.

See #10177 (comment) for more context.

How has this been tested?

I tested the behavior of the components in a default WP install using the Docker container provided by the install/setup script. I ran the test suite using npm test and updated the snapshot tests to match the new level of HOC wrappers.

I have dragged an image into the DropZone and used the MediaUploader to create an image block for the Image, Gallery, Cover, and File blocks.

Apologies for the poorly-named branch, I missed the instructions on branch naming 🤦‍♂️

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@kadamwhite
Copy link
Contributor

Thanks so much for putting this together, @mikeselander, it will be great to get this in to 4.2 if we can. Since I’m part of the team which asked for this change I’ll defer to @gziolo or others for a more impartial opinion; this looks like a positive change to me though!

@@ -162,4 +165,4 @@ export default compose(
getClientIdsOfDescendants,
};
} )
)( BlockDropZone );
)( withFilters( 'editor.BlockDropZone' )( BlockDropZone ) );
Copy link
Member

Choose a reason for hiding this comment

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

This should be moved into compose call:

   ...
   withFilters( 'editor.BlockDropZone' )
)( BlockDropZone );

@gziolo gziolo added the [Feature] Extensibility The ability to extend blocks or the editing experience label Oct 29, 2018
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

In general, I'm fine with the idea as proposed. I left one comment related to the implementation as it can be slightly refactored to take advantage of the existing compose helper function call.

We should add documentation to leave some examples how those filters can be used. See prior art by @adamsilverstein https://github.com/WordPress/gutenberg/tree/master/packages/editor/src/components/post-featured-image#postfeaturedimage or by @dsawardekar https://github.com/WordPress/gutenberg/blob/master/packages/editor/src/components/post-taxonomies/README.md.

If documentation is ready by tomorrow we will include it in 4.2.

@gziolo gziolo added the [Package] Editor /packages/editor label Oct 29, 2018
@mikeselander
Copy link
Contributor Author

Thanks for the feedback @gziolo! I've addressed the Compose issue and added some very simple documentation so that a developer at least has some example of how to make use of the filter. Let me know if anything else is needed!

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

All good now. Thank you for adding documentation and opening another extensibility points 🙇

@gziolo gziolo added this to the 4.2 milestone Oct 30, 2018
@gziolo gziolo merged commit fe51bd0 into WordPress:master Oct 30, 2018
@ellatrix
Copy link
Member

ellatrix commented Jan 7, 2020

@mikeselander @kadamwhite Out of curiosity, what are you replacing the drop zones with?

@mikeselander mikeselander deleted the filter-media-components branch January 7, 2020 14:50
@mikeselander
Copy link
Contributor Author

@ellatrix it's been over a year so I'm not 100% sure what we did here. iirc, we replaced DropZone with a pass-through component that did nothing to prevent any users from thinking they could upload any new media using the DropZone. This particular use case was integrating a 3rd-party DAM and all media was to be managed within the DAM so nothing could be added via WordPress.

@ellatrix
Copy link
Member

ellatrix commented Jan 7, 2020

@mikeselander Interesting. So you were preventing any media to be dropped in the editor? Or you did some custom handling of the media? The drop zone also handles drag and drop of blocks. Did you dropping those?

@mikeselander
Copy link
Contributor Author

So you were preventing any media to be dropped in the editor?

Yes

Or you did some custom handling of the media?

Also yes, we put additional filters in place in the media upload flow and the media library models that prevented uploading in different views.

The drop zone also handles drag and drop of blocks. Did you dropping those?

No, I believe that we allowed drag-and-drop of blocks around. This was a final task for a client and they moved the repo on to their own organization which we don't have access to after these changes so I'm unsure how they're handling this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Extensibility The ability to extend blocks or the editing experience [Package] Editor /packages/editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make All Components Package Elements Consistenly Filterable
4 participants