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

Add select box to choose logo size on email tab in settings #1159

Closed
wants to merge 2 commits into from

Conversation

philwp
Copy link

@philwp philwp commented Oct 27, 2016

Description

When adding a logo image in the 'emails' settings tab you cannot select which size image you want to display. I added a select box which uses the native WP image sizes to control the image size displayed in the email.

How Has This Been Tested?

I used multiple images testing all sizes in the email preview view. I also tested using a URL from an image hosted on another site.

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

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

$header_img = give_get_option( 'email_logo', '' );
$header_img_id = give_get_option( 'email_logo_id', '' );
$logo_size = give_get_option( 'email_logo_size', '' );
$header_img = $header_img_id == '' ? give_get_option( 'email_logo' ) : wp_get_attachment_image_src( $header_img_id, $logo_size )[0];
Copy link
Collaborator

@ravinderk ravinderk Oct 27, 2016

Choose a reason for hiding this comment

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

@philwp please validate wp_get_attachment_image_src before getting first element of array because this function can be return false.

for ref: https://developer.wordpress.org/reference/functions/wp_get_attachment_image_src/#return

@@ -543,6 +543,19 @@ public function give_settings( $active_tab ) {
'type' => 'file'
),
array(
'id' => 'email_logo_size',
Copy link
Collaborator

Choose a reason for hiding this comment

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

@philwp nowadays emails are responsive, so I did not think to give the admin the option to select logo size has more benefits than giving admin an option for email logo, so admin has full freedom to select upload their own size.

Copy link
Author

Choose a reason for hiding this comment

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

@ravinderk I ran into an issue where the image that one of my clients wanted to use was too large to look good in an email. The image was over 500px wide and I needed to manually enter the path to a smaller version of it.

Copy link
Collaborator

@ravinderk ravinderk Oct 27, 2016

Choose a reason for hiding this comment

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

@philwp It seems the most specific problem then general. It is easy to crop and upload an image and then assign as email logo.

@DevinWalker What do you think?

@DevinWalker
Copy link
Member

@ravinderk I personally have had the issue of uploading an image that is too large and felt frustrated by that. Personally, I like this field. @mathetos @kevinwhoffman what do you think?

@DevinWalker
Copy link
Member

@philwp can you switch this over to the release/1.6.4 branch?

@philwp philwp changed the base branch from master to release/1.6.4 October 27, 2016 18:27
@kevinwhoffman
Copy link
Contributor

I just tested and it is a nice addition especially for users who may already have a high-resolution logo uploaded and lack the graphics editing skills to create a specially sized logo just for emails.

With that said, we should also have a max-width style applied to that image in the event that an oversized image is selected.

@DevinWalker
Copy link
Member

@kevinwhoffman I agree on the max-width style for the image. I just set a huge image and tested and it looked pretty terrible.

@ravinderk
Copy link
Collaborator

@DevinWalker I already added max-width into release/1.8 with new setting api but I think i is too late.
I also experimented with adding images sizes model. Let me know what do you think.

screen shot 2016-10-28 at 1 03 46 am

@DevinWalker
Copy link
Member

Ah, I see @ravinderk do in release/1.8 the modal has a size select field. Is that correct?

@ravinderk
Copy link
Collaborator

ravinderk commented Oct 27, 2016

@DevinWalker I developed model now but max width already exists. Let me know if you are good with this model , I will create pr then

@ravinderk ravinderk mentioned this pull request Oct 28, 2016
3 tasks
@DevinWalker
Copy link
Member

Thanks @philwp we've merged #1168 thanks to you. Good idea. 👍

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.

None yet

4 participants