Skip to content

Integrate dominiant color with the media library#587

Closed
adamsilverstein wants to merge 10 commits intoWordPress:trunkfrom
adamsilverstein:add/dominant-color-media-library
Closed

Integrate dominiant color with the media library#587
adamsilverstein wants to merge 10 commits intoWordPress:trunkfrom
adamsilverstein:add/dominant-color-media-library

Conversation

@adamsilverstein
Copy link
Member

@adamsilverstein adamsilverstein commented Nov 29, 2022

Summary

Fixes #354

Relevant technical choices

  • In order to modify the templates used by core, the plugin unhooks the core templates and adds a modified copy. All code from L465 on are directly from core, with the addition of some inline styles (L491) and styles added to the HTML (L877). The specific template changes can also be reviewed here.
  • Core doesn't provide a way to filter or modify these templates, so the only approach to adding this is to replace them entirely. If/when this feature is merged to core, the changes would be much smaller

Screencast:

media-library-placeholders.mp4

Checklist

  • PR has either [Focus] or Infrastructure label.
  • PR has a [Type] label.
  • PR has a milestone or the no milestone label.

@pbearne pbearne self-requested a review December 6, 2022 19:48
@mxbclang mxbclang added [Focus] Images Needs Dev Anything that requires development (e.g. a pull request) labels Dec 13, 2022
@mxbclang mxbclang linked an issue Dec 13, 2022 that may be closed by this pull request
@mxbclang mxbclang added [Type] Enhancement A suggestion for improvement of an existing feature [Plugin] Image Placeholders Issues for the Image Placeholders plugin (formerly Dominant Color Images) and removed Needs Dev Anything that requires development (e.g. a pull request) labels Dec 13, 2022
Copy link
Contributor

@pbearne pbearne left a comment

Choose a reason for hiding this comment

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

don't like having to have all the duplicate code in the plugin but is the only way

@pbearne pbearne added this to the 1.8.0 milestone Dec 13, 2022
@felixarntz
Copy link
Member

This is still in draft stage and not ready, therefore punting it to 1.9.0 for now.

@felixarntz felixarntz modified the milestones: 1.8.0, 1.9.0 Dec 14, 2022
Copy link

@Leeroy28 Leeroy28 left a comment

Choose a reason for hiding this comment

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

Need fix update

@adamsilverstein
Copy link
Member Author

don't like having to have all the duplicate code in the plugin but is the only way

👍🏼 agree, although feel it is ok for the plugin, proposing this in core would be a much smaller change.

@adamsilverstein
Copy link
Member Author

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.

@adamsilverstein
Copy link
Member Author

@pbearne this is ready for another review/testing

@adamsilverstein adamsilverstein marked this pull request as ready for review December 20, 2022 17:04
@adamsilverstein adamsilverstein added the no milestone PRs that do not have a defined milestone for release label Dec 20, 2022
@pbearne
Copy link
Contributor

pbearne commented Dec 21, 2022

@adamsilverstein thanks for doing this

Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

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

Great work @adamsilverstein Left some nit-pick feedback.

@adamsilverstein
Copy link
Member Author

Thanks for the review and feedback @mukeshpanchal27 - I have applied your suggestions to the PR

Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

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

Thanks @adamsilverstein, The changes look good to me. I left some final feedback and then it was ready for merge.

@mukeshpanchal27 mukeshpanchal27 removed the no milestone PRs that do not have a defined milestone for release label Jan 11, 2023
@mukeshpanchal27
Copy link
Member

@felixarntz When you get time today, could you please review it so we can merge it for 1.9.0? Thanks!

@felixarntz
Copy link
Member

@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.

@felixarntz felixarntz modified the milestones: 1.9.0, 2.0.0 Jan 11, 2023
@adamsilverstein
Copy link
Member Author

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.

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

@felixarntz
Copy link
Member

@adamsilverstein

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.

@adamsilverstein
Copy link
Member Author

Makes sense. I guess one possibly for core would be adding a hook to make the media templates more customizable.

@felixarntz felixarntz removed this from the 2.0.0 milestone Feb 11, 2023
@felixarntz
Copy link
Member

Closing this for now as in the current stage there is no reasonable path forward for this to be in the plugin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Plugin] Image Placeholders Issues for the Image Placeholders plugin (formerly Dominant Color Images) [Type] Enhancement A suggestion for improvement of an existing feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants