Integrate dominiant color with the media library#587
Integrate dominiant color with the media library#587adamsilverstein wants to merge 10 commits intoWordPress:trunkfrom
Conversation
pbearne
left a comment
There was a problem hiding this comment.
don't like having to have all the duplicate code in the plugin but is the only way
|
This is still in draft stage and not ready, therefore punting it to 1.9.0 for now. |
👍🏼 agree, although feel it is ok for the plugin, proposing this in core would be a much smaller change. |
|
This now works correctly, thanks for the pointer @pbearne. I attached a screencast to the description. I noticed this also works during uploads, but only after image processing on the server (while the thumbnail loads). Even better would be a preview in the browser on upload: Gutenberg does this already. |
|
@pbearne this is ready for another review/testing |
|
@adamsilverstein thanks for doing this |
There was a problem hiding this comment.
Great work @adamsilverstein Left some nit-pick feedback.
Co-authored-by: Mukesh Panchal <[email protected]>
|
Thanks for the review and feedback @mukeshpanchal27 - I have applied your suggestions to the PR |
mukeshpanchal27
left a comment
There was a problem hiding this comment.
Thanks @adamsilverstein, The changes look good to me. I left some final feedback and then it was ready for merge.
|
@felixarntz When you get time today, could you please review it so we can merge it for |
|
@mukeshpanchal27 @adamsilverstein @pbearne I think for such a massive PR it's a bit late now. I understand that most of this code is duplicated from WordPress core, but that in itself is not great. Yes, it works, but it creates a maintenance nightmare. How are we able to keep track if anything in that part of core changes? I personally am not a fan of this approach, though I am aware that there's probably no other way to do it from a plugin. With that said, I don't know whether there's a point in further enhancing this module with additional functionality, when there is still a debate on the very foundation of the module and whether it makes sense for core. For something like this here, I would advise creating a WordPress core draft pull request, which allows testing this without the need for 100s of lines of duplicate code. |
Co-authored-by: Mukesh Panchal <[email protected]>
Co-authored-by: Mukesh Panchal <[email protected]>
This was the approach in the original PR Paul opened: https://github.com/WordPress/wordpress-develop/compare/trunk...pbearne:wordpress-develop:use_dominat_color_in_media_libary?expand=1 I don't think we can add this to core by itself though. If we want to test this approach in the plugin without core changes, we need to include a copy of the (modified) media template |
I understand. However as mentioned above that creates a maintenance nightmare that I don't think is justified. The dominant color feature in itself does still not have buy in from the core developers, and there have been, to my knowledge, little attempt to address the pushback on its original feature proposal. So adding it to the media library is going a step too far IMO, based on where the overall feature is at. If it was straightforward from a plugin, surely we could add it. But the limitations of missing hooks in that part of WordPress core to me make this a no-go. If at some point the feature has merit to move forward, we can always open a core PR with these changes to implement this in a more reasonable manner. |
|
Makes sense. I guess one possibly for core would be adding a hook to make the media templates more customizable. |
|
Closing this for now as in the current stage there is no reasonable path forward for this to be in the plugin. |
Summary
Fixes #354
Relevant technical choices
Screencast:
media-library-placeholders.mp4
Checklist
[Focus]orInfrastructurelabel.[Type]label.no milestonelabel.