Skip to content

First commit for dominant color core code#2906

Open
pbearne wants to merge 57 commits into
WordPress:trunkfrom
pbearne:dominant-color-core
Open

First commit for dominant color core code#2906
pbearne wants to merge 57 commits into
WordPress:trunkfrom
pbearne:dominant-color-core

Conversation

@pbearne
Copy link
Copy Markdown

@pbearne pbearne commented Jun 28, 2022

*
* @return string|WP_Error Dominant hex color string, or an error on failure.
*/
public function get_dominant_color() {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Need to add these methods to the WP_Image_Editor class.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These will both need to return WP_Error, with an a message saying these functions are not implemented.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Second thoughts maybe this is not the best idea. Let look at WordPress/performance#404

Comment thread src/wp-includes/media.php
* @param int $attachment_id The attachment ID.
* @return array $metadata The attachment metadata.
*/
function dominant_color_metadata( $metadata, $attachment_id ) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can just add this directly there

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

filter removed and calling function directly

Comment thread src/wp-includes/media.php
* @param object $attachment Image attachment post.
* @return mixed $attr Attributes for the image markup.
*/
function dominant_color_update_attachment_image_attributes( $attr, $attachment ) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can add these directly herer -

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

filter removed and calling function directly

Comment thread src/wp-includes/media.php Outdated
* @param int $attachment_id The attachment ID.
* @return string image tag
*/
function dominant_color_img_tag_add_dominant_color( $filtered_image, $context, $attachment_id ) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's not filter and manaully add this in here -

Comment thread src/wp-includes/media.php Outdated
Comment thread src/wp-includes/media.php Outdated
*/
function _dominant_color_get_dominant_color_data( $attachment_id ) {
if ( ! wp_attachment_is_image( $attachment_id ) ) {
return new WP_Error( 'no_image_found', __( 'Unable to load image.', 'performance-lab' ) );
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Remove all the , 'performance-lab' references.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Still seeing performance lab

/**
* Get dominant color from a file.
*
* @since 1.2.0
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Change all references to 1.2.0 to 6.1

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done

pbearne and others added 10 commits July 12, 2022 11:13
Co-authored-by: Jonny Harris <spacedmonkey@users.noreply.github.com>
Co-authored-by: Jonny Harris <spacedmonkey@users.noreply.github.com>
…-test.php

Co-authored-by: Jonny Harris <spacedmonkey@users.noreply.github.com>
Co-authored-by: Jonny Harris <spacedmonkey@users.noreply.github.com>
Co-authored-by: Jonny Harris <spacedmonkey@users.noreply.github.com>
@spacedmonkey
Copy link
Copy Markdown
Member

@pbearne Coding standard issues.

Comment thread src/wp-includes/class-wp-image-editor-imagick.php
Comment thread src/wp-includes/media.php Outdated
Comment thread src/wp-includes/media.php Outdated
*/
function _dominant_color_get_dominant_color_data( $attachment_id ) {
if ( ! wp_attachment_is_image( $attachment_id ) ) {
return new WP_Error( 'no_image_found', __( 'Unable to load image.', 'performance-lab' ) );
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Still seeing performance lab

Comment thread src/wp-includes/media.php Outdated
Comment thread src/wp-includes/media.php
* @param string $size Optional. Image size. Default 'thumbnail'.
* @return false|string Path to an image or false if not found.
*/
function wp_get_attachment_file_path( $attachment_id, $size = 'medium' ) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Mismatch between default value for $size and the PHPDoc (medium vs thumbnail)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good spot and thanks
Fixed

pbearne added 3 commits July 20, 2022 12:21
Fixed a mismatch between default value for $size and the PHPDoc (medium vs thumbnail)
reverted none related whitespace edit
revert whitespace  edit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants