(new) Improve the upsell button caret icon position in RTL languages #11398

afercia · 2018-10-24T11:05:44Z

Summary

This PR can be summarized in the following changelog entry:

N/A

Relevant technical choices:

introduces a new (left) caret icon
instead of using background images directly on the button, use the image as background of an inline <span> element so it behaves like inline content
refactors the CSS to use inline-flex so the text and the icon are better aligned vertically
the upsell button can now be used also without an icon (if necessary) so it's better reusable
reuses the yoast-button-upsell also in the Extensions page
adds the "new tab message" to all links that open in a new tab in the extensions page
incorporates the line-height fix from Fix upsell button line height #11334

Test instructions

The upsell button needs to be checked everywhere it appears, and also in the responsive view. As far as I know, it is used:

in the notice when trashing a published post
in the Help Center > Get support
in the sidebar in the admin pages
at the bottom of the admin pages: the upsell block
in the extensions page (SEO > Premium) also known as "licenses" page wp-admin/admin.php?page=wpseo_licenses
Additional keywords upsell modal
Additional keywords upsell section
Synonyms upsell modal

All these places need to be tested in LTR and RTL.

Example screenshot:

Fixes #11174

… page.

andizer

CR done. Found two minor things.

andizer · 2018-10-26T07:24:06Z

admin/class-premium-popup.php

@@ -77,8 +77,10 @@ public function get_premium_message( $popup = true ) {
 		$assets_uri = trailingslashit( plugin_dir_url( WPSEO_FILE ) );

 		/* translators: %s expands to Yoast SEO Premium */
-		$cta_text = sprintf( __( 'Get %s', 'wordpress-seo' ), 'Yoast SEO Premium' );
-		$classes  = '';
+		$cta_text        = sprintf( __( 'Get %s', 'wordpress-seo' ), 'Yoast SEO Premium' );


Shouldn't we do esc_html__ here like https://github.com/Yoast/wordpress-seo/pull/11398/files#diff-298aab7b9ea9a84b3500fdad7618ca6dR63

andizer · 2018-10-26T07:24:39Z

admin/class-premium-popup.php

-		$cta_text = sprintf( __( 'Get %s', 'wordpress-seo' ), 'Yoast SEO Premium' );
-		$classes  = '';
+		$cta_text        = sprintf( __( 'Get %s', 'wordpress-seo' ), 'Yoast SEO Premium' );
+		$new_tab_message = '<span class="screen-reader-text">' . __( '(Opens in a new browser tab)', 'wordpress-seo' ) . '</span>';


See the comment above. On https://github.com/Yoast/wordpress-seo/pull/11398/files#diff-298aab7b9ea9a84b3500fdad7618ca6dR64 it is escaped

IreneStr · 2018-10-29T07:44:08Z

2nd CR 👍

IreneStr · 2018-10-29T08:21:45Z

Acceptance 🚧

The vertical alignment of the caret seems off in the redirect notice:

At the top on the premium page:

In the admin (both places):

But the alignment looks fine in the extension 'cards':

This button is a little strange (might be preexisting):

The same for the button in the help center in responsive mode (might be preexisting):

This might be preexisting, but in responsive mode (more specific iPhone 5/SE), this button overlaps the text:

The caret has gone missing in the:

Additional keywords upsell modal

Additional keywords upsell section

Synonyms upsell modal

All RTL-related things look good 👌

IreneStr · 2018-10-29T08:22:38Z

@afercia There are three more places:

Additional keywords upsell modal
Additional keywords upsell section
Synonyms upsell modal

I've added them to your PR description

afercia · 2018-10-29T10:24:05Z

Added the missing caret in the UpsellBox component button. However, I can't reproduce the misalignments screenshot'd above. A couple examples of what I see on Chrome 70:

afercia · 2018-10-29T10:42:59Z

Comparing side by side what my screenshots and the ones from @IreneStr I see the actual font used is different. It's very evident especially for some letters, see for example the shape of s, e, a below:

This seems to be caused by the font stack used on the button:
font-family: "Open Sans", Arial, sans-serif;

@IreneStr do you have any "Open Sans" or "Arial" fonts installed in your system fonts? On macOS you can check the fonts in the "Font Book" app.

I think the only thing we can try is to mitigate the problem tweaking the line-height.

afercia · 2018-10-29T10:44:21Z

Aside: why are we using font-family: "Open Sans", Arial, sans-serif;?

afercia · 2018-10-29T11:57:43Z

I've installed in my system fonts Open Sans from Google fonts and I can reproduce the misalignment now. Hard to fix. The root issue is we're using a font that is not installed by default on maOS nor Windows. (not sure about *nix distros).

afercia · 2018-10-29T12:47:51Z

I've committed a temporary fix to not use "Open Sans" on the Upsell button. We should consider to remove any non-default-system-font, will create a new issue.

Dieterrr · 2018-10-29T15:52:12Z

CR done 👍. I agree we need a separate issue to have a more consistent approach to the use of fonts. I am not sure removing a troublesome font is the best solution.
What I do understand from the design department is that "Open Sans" was choses because it's look is close to the system fonts of both Mac and Windows. On the Yoast website it is included. I do not know why it is not included in the plugin.
Acceptance: The caret is no longer missing in Additional keywords upsell modal,
Additional keywords upsell section, Synonyms upsell modal & Synonyms upsell section.
However, in the modal and in the "cards" the alignment is still off, now it looks a bit too far down:

Scientific proof:

Also, although I'm not sure if this is part of the issue, the button can still overlap with the text:

Perhaps we should discuss this issue at the standup because I can imagine this micro-adjustment of the Caret - which behaves different on different machines - can become frustrating.

afercia · 2018-10-29T16:46:53Z

A bit of historical background can help.
WordPress used to use Open Sans. It was removed in WordPress 4.6 in favor of system fonts. See also #4582

In the plugin, the font stack that uses Open Sans comes from the inclusion of files that were originally developed for yoast.com. However, the plugin is not using Google Fonts to actually include Open Sans so these font stacks are pointless, including the one that uses Merriweather.

Additionally, they're not only pointless but they create problems when users have manually installed these fonts (for any reasons) in their system.

I've removed the 1 pixel top margin from the caret. Notice the rendering is different in Firefox, and the alignment won't look perfect. Also, the icon doesn't help because it has white space around the shape.

The button overlapping content is a pre-existing unrelated issue due to the button container implementation. I'll see what can be done.

@Dieter have you checked if you have "Open Sans" installed in your system fonts and, if so, have you uninstalled as I've recommended on Slack? In your screenshot I see the purple title has a lighter weight than expected: it should be bolder:

While your one is clearly different:

afercia · 2018-10-29T17:55:26Z

I'm boy-scouting the overlapping issue by removing the absolute positioning of the button container. That didn't take into account the case of taller buttons when their content goes in two or more lines.

The "cards" are now flex items of their container an also flex containers for their content, to the content can be layout vertically using flex-direction: column.

cards still have the same height when in the same row
buttons are pushed to the bottom as before, even if the card content is shorter
buttons with two or more lines of text don't overlap any longer
also fixes the buttons min-height hack for IE11, that never worked

Screenshot with an intentionally exaggerated edge case:

IreneStr · 2018-10-30T11:25:33Z

CR done 👍
The font-related things will be addressed in a separate issue.

abotteram · 2018-10-31T08:33:03Z

Acceptance 👍

afercia added 6 commits October 24, 2018 12:23

Improve hte upell button caret icon position.

616b786

Reuse the yoast-button-upsell class in the extensions page.

f590017

Add new tab message to links that open in a new tab in the extensions…

1d03d24

… page.

Make phpcs happy.

727eb61

Fix the upsell admin block button.

4ec8182

Incoporate line-height fix from #11334.

db9ea2c

afercia mentioned this pull request Oct 24, 2018

Improve the upsell button caret icon position in RTL languages #11335

Closed

andizer suggested changes Oct 26, 2018

View reviewed changes

Add missing escaping.

c705ee2

Add caret icon to the UpsellBox component button.

4c68720

Don't use Open Sans.

5fdcb9a

Remove caret top margin.

64498bf

afercia added 2 commits October 29, 2018 18:10

Use flexbox for the Premium cards content.

ad3e895

Fix flexbox and button min-height hack for IE11.

5154db1

abotteram merged commit 9c97a74 into trunk Oct 31, 2018

abotteram deleted the 11174-upsell-button-caret-icon-rtl-new branch October 31, 2018 08:33

abotteram added this to the 9.2 milestone Oct 31, 2018

hedgefield mentioned this pull request Oct 31, 2018

Define default font stack Yoast/yoast-components#772

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(new) Improve the upsell button caret icon position in RTL languages #11398

(new) Improve the upsell button caret icon position in RTL languages #11398

afercia commented Oct 24, 2018 •

edited by IreneStr

andizer left a comment

andizer Oct 26, 2018

andizer Oct 26, 2018

IreneStr commented Oct 29, 2018

IreneStr commented Oct 29, 2018 •

edited

IreneStr commented Oct 29, 2018

afercia commented Oct 29, 2018

afercia commented Oct 29, 2018

afercia commented Oct 29, 2018

afercia commented Oct 29, 2018

afercia commented Oct 29, 2018

Dieterrr commented Oct 29, 2018

afercia commented Oct 29, 2018

afercia commented Oct 29, 2018

IreneStr commented Oct 30, 2018

abotteram commented Oct 31, 2018

(new) Improve the upsell button caret icon position in RTL languages #11398

(new) Improve the upsell button caret icon position in RTL languages #11398

Conversation

afercia commented Oct 24, 2018 • edited by IreneStr

Summary

Relevant technical choices:

Test instructions

andizer left a comment

Choose a reason for hiding this comment

andizer Oct 26, 2018

Choose a reason for hiding this comment

andizer Oct 26, 2018

Choose a reason for hiding this comment

IreneStr commented Oct 29, 2018

IreneStr commented Oct 29, 2018 • edited

IreneStr commented Oct 29, 2018

afercia commented Oct 29, 2018

afercia commented Oct 29, 2018

afercia commented Oct 29, 2018

afercia commented Oct 29, 2018

afercia commented Oct 29, 2018

Dieterrr commented Oct 29, 2018

afercia commented Oct 29, 2018

afercia commented Oct 29, 2018

IreneStr commented Oct 30, 2018

abotteram commented Oct 31, 2018

afercia commented Oct 24, 2018 •

edited by IreneStr

IreneStr commented Oct 29, 2018 •

edited