Converted snippet preview mode buttons to radio buttons #376

hwinne · 2019-10-15T15:06:04Z

Summary

This issue has a PR in both the javascript and wordpress-seo repo.

Removed the mobile and desktop mode buttons, added radio buttons. Changed the title from Snippet Preview to Google search result preview (see Yoast/wordpress-seo#13649). Moved the buttons above the Snippet Preview.

This PR can be summarized in the following changelog entry:

[wordpress-seo] Enhances the Snippet Preview in the metabox by using radio buttons.
[@yoast/search-metadata-previews] Enhances the Snippet Editor's Mode Switcher component by using radio buttons.
[@yoast/components] Adds className attribute to the Input component.
[@yoast/components] Adds className and optionalAttributes attributes to the Label component.

Relevant technical choices:

Changed regular mode buttons to radio buttons
Moved mode buttons above the Snippet Preview
Changed the Snippet Preview Title (in ).

Test instructions

This PR can be tested by following these steps:

The snippet preview should look like the provided screenshots down below.
- Test on all pages the Snippet Editor is used (such as: Posts, Pages, Tags, Categories, etc...)
The snipper preview should have the title "Google search result preview"
- In the Metabox, under a post
- In the Gutenberg sidebar menu.
- In the Snippet Preview, when clicked from the Gutenberg sidebar menu.
The mobile/desktop mode should be toggled, when the radio buttons are toggled.

Screenshots

Under a post:
In the Yoast sidebar in Gutenberg:
When the Snippet Preview is accessed from the Yoast sidebar in Gutenberg:

Impact check

This PR affects the following parts of the plugin, which may require extra testing:
- Yoast Sidebar in the Gutenberg editor
- Any page that uses the Snippet Preview in the Metabox

UI changes

This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Quality assurance

I have tested this code to the best of my abilities
I have added unittests to verify the code works as intended

Fixes #Yoast/wordpress-seo#13192

…ttons

hwinne · 2019-10-15T15:09:31Z

🚧 The tests should be updated.

…192-ux-improvements-to-snippet-preview

afercia · 2019-10-28T10:35:35Z

packages/search-metadata-previews/src/snippet-editor/ModeSwitcher.js

+		<SwitcherTitle>{ __( "Preview as", "yoast-components" ) + ":" }</SwitcherTitle>
+		<ModeLabel>
+			<ModeRadio
+				onChange={ () => onChange( MODE_MOBILE ) }


Pre-existing ESLint warning: JSX props should not use arrow functions

afercia · 2019-10-28T10:35:42Z

packages/search-metadata-previews/src/snippet-editor/ModeSwitcher.js

+		</ModeLabel>
+		<ModeLabel>
+			<ModeRadio
+				onChange={ () => onChange( MODE_DESKTOP ) }


Pre-existing ESLint warning: JSX props should not use arrow functions

afercia · 2019-10-28T10:35:54Z

packages/search-metadata-previews/src/snippet-editor/SnippetEditor.js

@@ -528,6 +528,8 @@ class SnippetEditor extends React.Component {
 		return (
 			<ErrorBoundary>
 				<div>
+					<ModeSwitcher onChange={ ( newMode ) => onChange( "mode", newMode ) } active={ mode } />


Pre-existing ESLint warning: JSX props should not use arrow functions

afercia · 2019-10-28T10:40:46Z

packages/search-metadata-previews/src/snippet-editor/ModeSwitcher.js

-const MobileButton = styled( SwitcherButton )`
-	border-radius: 3px 0 0 3px;
+const ModeLabel = styled.label`
+	font: 400 14px/24px "Open Sans",sans-serif;


Not sure we should use "Open Sans",sans-serif. Unless I'm missing something I think we normally use "font-family: inherit" and set the other values separately.

afercia · 2019-10-28T10:41:59Z

packages/search-metadata-previews/src/snippet-editor/ModeSwitcher.js

-	border-radius: 3px 0 0 3px;
+const ModeLabel = styled.label`
+	font: 400 14px/24px "Open Sans",sans-serif;
+	margin-right: 18px;


Why 18px? I guess this should use our 8 pixels grid thus 16px would be a better value.
More importantly: this needs to be reverted for RTL.

afercia · 2019-10-28T10:42:48Z

packages/search-metadata-previews/src/snippet-editor/ModeSwitcher.js

-const DesktopButton = styled( SwitcherButton )`
-	border-radius: 0 3px 3px 0;
+const ModeRadio = styled.input`
+	margin-right: 10px;


Why 10px? I guess this should use our 8 pixels grid thus 8px would be a better value.
More importantly: this needs to be reverted for RTL.

afercia · 2019-10-28T10:43:46Z

packages/search-metadata-previews/src/snippet-editor/ModeSwitcher.js

-		color: ${ colors.$color_snippet_focus };
-		box-shadow: none;
-	}
+const Switcher = styled.div`


This should be a <fieldset> element to properly group the radio buttons.

afercia · 2019-10-28T10:44:24Z

packages/search-metadata-previews/src/snippet-editor/ModeSwitcher.js

-	border-radius: 4px;
-	background-color: #f7f7f7;
-	vertical-align: top;
+const SwitcherTitle = styled.h4`


This should be the <legend> of the <fieldset> element.
(Also, using a h4 skips one level within the meta box, as the previous heading is a h2)

afercia · 2019-10-28T10:47:02Z

packages/search-metadata-previews/src/snippet-editor/ModeSwitcher.js

-		</DesktopButton>
-	</Switcher>;
+	return ( <Switcher>
+		<SwitcherTitle>{ __( "Preview as", "yoast-components" ) + ":" }</SwitcherTitle>


I know this + ":" pattern is used in a few other places but I'd reconsider it. What if other languages don't use colons? Translator wouldn't be able to remove it. I'd tend to think the colon should be part of the translatable string.

afercia · 2019-10-28T10:47:54Z

packages/search-metadata-previews/src/snippet-editor/ModeSwitcher.js

+			<ModeRadio
+				onChange={ () => onChange( MODE_MOBILE ) }
+				checked={ active === MODE_MOBILE }
+				aria-pressed={ active === MODE_MOBILE }


Please remove aria-pressed: it was OK for the previous buttons but it's not appropriate for radio buttons.

afercia · 2019-10-28T10:48:14Z

packages/search-metadata-previews/src/snippet-editor/ModeSwitcher.js

+			<ModeRadio
+				onChange={ () => onChange( MODE_DESKTOP ) }
+				checked={ active === MODE_DESKTOP }
+				aria-pressed={ active === MODE_DESKTOP }


Please remove aria-pressed: it was OK for the previous buttons but it's not appropriate for radio buttons.

afercia · 2019-10-28T10:49:52Z

packages/search-metadata-previews/src/snippet-editor/ModeSwitcher.js

-	</Switcher>;
+	return ( <Switcher>
+		<SwitcherTitle>{ __( "Preview as", "yoast-components" ) + ":" }</SwitcherTitle>
+		<ModeLabel>


In other implementations we don't use wrapping labels. Instead, we use separate label and input, associated with for and id attributes. See for example the Configuration wizard. That's the recommended way to go as some assistive technologies don't work well with wrapping labels.

Also, I'd consider to use the existing Label component.

afercia · 2019-10-28T10:51:04Z

packages/search-metadata-previews/src/snippet-editor/ModeSwitcher.js

+	return ( <Switcher>
+		<SwitcherTitle>{ __( "Preview as", "yoast-components" ) + ":" }</SwitcherTitle>
+		<ModeLabel>
+			<ModeRadio


I'd consider to use the existing Input component (see the Configuration wizard for usage example)

afercia · 2019-10-28T10:51:42Z

packages/search-metadata-previews/src/snippet-editor/ModeSwitcher.js

+			<span>{ __( "Mobile result", "yoast-components" ) }</span>
+		</ModeLabel>
+		<ModeLabel>
+			<ModeRadio


Same as above: consider to use the existing Label and Input components.

afercia · 2019-10-28T10:55:49Z

packages/search-metadata-previews/src/snippet-editor/ModeSwitcher.js

+				name="screen"
+				value="desktop"
+			/>
+			<span>{ __( "Desktop result", "yoast-components" ) }</span>


Unless I'm missing something and it actually serves some purpose, please remove this <span> element.

afercia · 2019-10-28T10:56:10Z

packages/search-metadata-previews/src/snippet-editor/ModeSwitcher.js

+				name="screen"
+				value="mobile"
+			/>
+			<span>{ __( "Mobile result", "yoast-components" ) }</span>


Unless I'm missing something and it actually serves some purpose, please remove this <span> element.

afercia · 2019-10-28T13:15:31Z

packages/search-metadata-previews/src/snippet-editor/ModeSwitcher.js

+	font: 400 14px/24px "Open Sans",sans-serif;
+	margin-right: 18px;
+	cursor: pointer;
+	color: #3c4043;


This color is a Google color, only used for the snippet URL and the description. We shouldn't use it for our UI.

afercia · 2019-10-28T13:15:39Z

packages/search-metadata-previews/src/snippet-editor/ModeSwitcher.js

-	vertical-align: top;
+const SwitcherTitle = styled.h4`
+	margin: .5em 0;
+	color: #3c4043;


This color is a Google color, only used for the snippet URL and the description. We shouldn't use it for our UI.

afercia · 2019-10-28T15:27:08Z

CR 🚧

Some considerations to address, see comments. Most notably:

styles for RTL
better semantics: group the radio buttons with fieldset + legend

afercia · 2019-10-28T15:48:45Z

Changes in the last commit:

used the existing Label and Input components
modified Label and Input to get the className passed by styled-components
please double check the change to Label optionalAttributes: may need adjustments
used a fieldset + legend elements to group the radio buttons
added styles for RTL

afercia · 2019-10-28T15:51:26Z

Note: RTL testing in the standalone demo appears to be broken for some reason.

Thus RTL needs to be tested directly in the plugin. Screenshots:

LTR:

RTL:

afercia · 2019-10-30T12:02:35Z

Note: very recent CSS changes in Gutenberg that were merged in WordPress core as part of the RC 3 broke the radio buttons layout within the meta boxes area. The radio buttons "blue dot" is now completely misplaced. See:

WordPress/gutenberg#18181
https://core.trac.wordpress.org/ticket/48466

This should be fixed upstream, hopefully as soon as possible because it's a regression in WP trunk.

igorschoester · 2019-12-09T15:55:55Z

Acceptance 🚧

One error found:
When using the sidebar, the Google preview modal closes when you click on a radio button.

…192-ux-improvements-to-snippet-preview

…thub.com/Yoast/javascript into 13192-ux-improvements-to-snippet-preview

hwinne · 2019-12-16T14:49:43Z

As Igor mentioned, the Google Preview modal closed when clicking the label of the radio button. I added onClick={ event.preventDefault } to the labels, so the label does not execute its default actions. @afercia could you please confirm this is the correct way of doing it?

When clicking the label it should still select the corresponding radio button.

When testing this, I had a strange bug where the fix would stay intact, even when I removed onClick={ event.preventDefault } (Meaning the modal would not close). Please make sure clicking the label will close the modal when removing said code.

abotteram

CR 🚧

I don't know why the modal closes if you click the label, but this is not the right way to fix it I think.

packages/search-metadata-previews/src/snippet-editor/ModeSwitcher.js

packages/components/src/Label.js

IreneStr · 2019-12-17T14:59:38Z

Acceptance 👍

afercia · 2019-12-23T14:02:29Z

I don't know why the modal closes if you click the label, but this is not the right way to fix it I think.

Yeah, this happened (only in Chrome) because of the duplicate IDs and I guess the Gutenmodal "click outside" being triggered. Didn't happen on Firefox.

Converted snippet preview desktop and mobile mode buttons to radio bu…

9b83933

…ttons

hwinne added UI change PRs that result in a change in the UI Package: search-metadata-previews changelog: enhancement Needs to be included in the 'Enhancements' category in the changelog labels Oct 15, 2019

hwinne mentioned this pull request Oct 15, 2019

Changed Snippet Preview title to Google search result preview Yoast/wordpress-seo#13649

Merged

4 tasks

Updated the snippet editor test and snapshot

748ae98

hwinne mentioned this pull request Oct 22, 2019

UX improvements to snippet preview Yoast/wordpress-seo#13192

Closed

Merge branch 'develop' of https://github.com/Yoast/javascript into 13…

13cc25c

…192-ux-improvements-to-snippet-preview

afercia reviewed Oct 28, 2019

View reviewed changes

afercia mentioned this pull request Oct 28, 2019

Internet Explorer 11: standalone demo app doesn't work #351

Open

afercia reviewed Oct 28, 2019

View reviewed changes

Address CR feedback.

9eee2e0

hwinne added 4 commits December 10, 2019 09:14

Merge branch 'develop' of https://github.com/Yoast/javascript into 13…

699d6b8

…192-ux-improvements-to-snippet-preview

Merge branch 'develop' into 13192-ux-improvements-to-snippet-preview

4c4c268

Merge branch '13192-ux-improvements-to-snippet-preview' of https://gi…

bdbf07f

…thub.com/Yoast/javascript into 13192-ux-improvements-to-snippet-preview

Added preventDefault so clicking the label doesn't close the modal

ec0028e

updated tests, put preventDefault in function handleLabelClick

66bd043

abotteram suggested changes Dec 17, 2019

View reviewed changes

Added unique ids in modeswitcher

257e522

abotteram approved these changes Dec 17, 2019

View reviewed changes

Updated snapshots

dff554f

IreneStr merged commit b6e4294 into develop Dec 17, 2019

IreneStr deleted the 13192-ux-improvements-to-snippet-preview branch December 17, 2019 15:00

IreneStr added this to the 12.9 milestone Dec 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Converted snippet preview mode buttons to radio buttons #376

Converted snippet preview mode buttons to radio buttons #376

hwinne commented Oct 15, 2019 •

edited by igorschoester

Loading

hwinne commented Oct 15, 2019

afercia Oct 28, 2019

afercia Oct 28, 2019

afercia Oct 28, 2019

afercia Oct 28, 2019

afercia Oct 28, 2019

afercia Oct 28, 2019

afercia Oct 28, 2019

afercia Oct 28, 2019 •

edited

Loading

afercia Oct 28, 2019

afercia Oct 28, 2019

afercia Oct 28, 2019

afercia Oct 28, 2019

afercia Oct 28, 2019

afercia Oct 28, 2019

afercia Oct 28, 2019

afercia Oct 28, 2019

afercia Oct 28, 2019

afercia Oct 28, 2019

afercia Oct 28, 2019

afercia commented Oct 28, 2019

afercia commented Oct 28, 2019

afercia commented Oct 28, 2019

afercia commented Oct 30, 2019

igorschoester commented Dec 9, 2019

hwinne commented Dec 16, 2019 •

edited

Loading

abotteram left a comment

IreneStr commented Dec 17, 2019

afercia commented Dec 23, 2019

Converted snippet preview mode buttons to radio buttons #376

Converted snippet preview mode buttons to radio buttons #376

Conversation

hwinne commented Oct 15, 2019 • edited by igorschoester Loading

Summary

Relevant technical choices:

Test instructions

Screenshots

Impact check

UI changes

Quality assurance

hwinne commented Oct 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afercia Oct 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afercia commented Oct 28, 2019

afercia commented Oct 28, 2019

afercia commented Oct 28, 2019

afercia commented Oct 30, 2019

igorschoester commented Dec 9, 2019

hwinne commented Dec 16, 2019 • edited Loading

abotteram left a comment

Choose a reason for hiding this comment

IreneStr commented Dec 17, 2019

afercia commented Dec 23, 2019

hwinne commented Oct 15, 2019 •

edited by igorschoester

Loading

afercia Oct 28, 2019 •

edited

Loading

hwinne commented Dec 16, 2019 •

edited

Loading