-
Notifications
You must be signed in to change notification settings - Fork 54
Converted snippet preview mode buttons to radio buttons #376
Conversation
🚧 The tests should be updated. |
…192-ux-improvements-to-snippet-preview
<SwitcherTitle>{ __( "Preview as", "yoast-components" ) + ":" }</SwitcherTitle> | ||
<ModeLabel> | ||
<ModeRadio | ||
onChange={ () => onChange( MODE_MOBILE ) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pre-existing ESLint warning: JSX props should not use arrow functions
</ModeLabel> | ||
<ModeLabel> | ||
<ModeRadio | ||
onChange={ () => onChange( MODE_DESKTOP ) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pre-existing ESLint warning: JSX props should not use arrow functions
@@ -528,6 +528,8 @@ class SnippetEditor extends React.Component { | |||
return ( | |||
<ErrorBoundary> | |||
<div> | |||
<ModeSwitcher onChange={ ( newMode ) => onChange( "mode", newMode ) } active={ mode } /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pre-existing ESLint warning: JSX props should not use arrow functions
const MobileButton = styled( SwitcherButton )` | ||
border-radius: 3px 0 0 3px; | ||
const ModeLabel = styled.label` | ||
font: 400 14px/24px "Open Sans",sans-serif; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
border-radius: 3px 0 0 3px; | ||
const ModeLabel = styled.label` | ||
font: 400 14px/24px "Open Sans",sans-serif; | ||
margin-right: 18px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
const DesktopButton = styled( SwitcherButton )` | ||
border-radius: 0 3px 3px 0; | ||
const ModeRadio = styled.input` | ||
margin-right: 10px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
color: ${ colors.$color_snippet_focus }; | ||
box-shadow: none; | ||
} | ||
const Switcher = styled.div` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a <fieldset>
element to properly group the radio buttons.
border-radius: 4px; | ||
background-color: #f7f7f7; | ||
vertical-align: top; | ||
const SwitcherTitle = styled.h4` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
</DesktopButton> | ||
</Switcher>; | ||
return ( <Switcher> | ||
<SwitcherTitle>{ __( "Preview as", "yoast-components" ) + ":" }</SwitcherTitle> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
<ModeRadio | ||
onChange={ () => onChange( MODE_MOBILE ) } | ||
checked={ active === MODE_MOBILE } | ||
aria-pressed={ active === MODE_MOBILE } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove aria-pressed
: it was OK for the previous buttons but it's not appropriate for radio buttons.
<ModeRadio | ||
onChange={ () => onChange( MODE_DESKTOP ) } | ||
checked={ active === MODE_DESKTOP } | ||
aria-pressed={ active === MODE_DESKTOP } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove aria-pressed
: it was OK for the previous buttons but it's not appropriate for radio buttons.
</Switcher>; | ||
return ( <Switcher> | ||
<SwitcherTitle>{ __( "Preview as", "yoast-components" ) + ":" }</SwitcherTitle> | ||
<ModeLabel> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I'd consider to use the existing Label
component.
return ( <Switcher> | ||
<SwitcherTitle>{ __( "Preview as", "yoast-components" ) + ":" }</SwitcherTitle> | ||
<ModeLabel> | ||
<ModeRadio |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd consider to use the existing Input
component (see the Configuration wizard for usage example)
<span>{ __( "Mobile result", "yoast-components" ) }</span> | ||
</ModeLabel> | ||
<ModeLabel> | ||
<ModeRadio |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: consider to use the existing Label
and Input
components.
name="screen" | ||
value="desktop" | ||
/> | ||
<span>{ __( "Desktop result", "yoast-components" ) }</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm missing something and it actually serves some purpose, please remove this <span>
element.
name="screen" | ||
value="mobile" | ||
/> | ||
<span>{ __( "Mobile result", "yoast-components" ) }</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm missing something and it actually serves some purpose, please remove this <span>
element.
font: 400 14px/24px "Open Sans",sans-serif; | ||
margin-right: 18px; | ||
cursor: pointer; | ||
color: #3c4043; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This color is a Google color, only used for the snippet URL and the description. We shouldn't use it for our UI.
vertical-align: top; | ||
const SwitcherTitle = styled.h4` | ||
margin: .5em 0; | ||
color: #3c4043; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This color is a Google color, only used for the snippet URL and the description. We shouldn't use it for our UI.
CR 🚧 Some considerations to address, see comments. Most notably:
|
Changes in the last commit:
|
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 This should be fixed upstream, hopefully as soon as possible because it's a regression in WP trunk. |
Acceptance 🚧 One error found: |
…192-ux-improvements-to-snippet-preview
…thub.com/Yoast/javascript into 13192-ux-improvements-to-snippet-preview
As Igor mentioned, the Google Preview modal closed when clicking the label of the radio button. I added 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
packages/search-metadata-previews/src/snippet-editor/ModeSwitcher.js
Outdated
Show resolved
Hide resolved
packages/search-metadata-previews/src/snippet-editor/ModeSwitcher.js
Outdated
Show resolved
Hide resolved
Acceptance 👍 |
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. |
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
toGoogle 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:
className
attribute to the Input component.className
andoptionalAttributes
attributes to the Label component.Relevant technical choices:
Test instructions
This PR can be tested by following these steps:
Screenshots
Under a post:
In the Yoast sidebar in Gutenberg:
When the Snippet Preview is accessed from the Yoast sidebar in Gutenberg:
Impact check
UI changes
Quality assurance
Fixes #Yoast/wordpress-seo#13192