10267 add cornerstone toggle to sidebar #668
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| import React from "react"; | ||
| import PropTypes from "prop-types"; | ||
| import styled from "styled-components"; | ||
| import Toggle from "../../Shared/components/Toggle"; | ||
| import { __ } from "@wordpress/i18n"; | ||
|
|
||
| const Cornerstone = styled.div` | ||
| display: flex; | ||
| justify-content: space-between; | ||
| align-items: center; | ||
| margin-top: 5px; | ||
|
|
||
| label { | ||
| margin-right: 10px; | ||
| flex-shrink: 0; | ||
| max-width: 75%; | ||
| } | ||
| `; | ||
|
|
||
| class CornerstoneToggle extends React.Component { | ||
| /** | ||
| * Renders the CornerstoneToggle component. | ||
| * | ||
| * @returns {ReactElement} the CornerstoneToggle component. | ||
| */ | ||
| render() { | ||
| return ( | ||
| <Cornerstone> | ||
| <Toggle ariaLabel="Mark this post as cornerstone content" id="Cornerstone Toggle" | ||
| labelText={__( "Mark this as cornerstone content.", "yoast-components" )} /> | ||
|
There was a problem hiding this comment. missing spaces between brackets There was a problem hiding this comment. On a side note, we shouldn't use mixed spaces and tabs for indentation |
||
| </Cornerstone> | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| CornerstoneToggle.propTypes = { | ||
|
There was a problem hiding this comment. We usually set defaults for PropTypes that are not required. |
||
| isEnabled: PropTypes.bool, | ||
| ariaLabel: PropTypes.string.isRequired, | ||
| onSetEnable: PropTypes.func, | ||
| disable: PropTypes.bool, | ||
| onToggleDisabled: PropTypes.func, | ||
| id: PropTypes.string, | ||
| labelText: PropTypes.string, | ||
| }; | ||
|
|
||
| export default CornerstoneToggle; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| import React from "react"; | ||
| import renderer from "react-test-renderer"; | ||
| import CornerstoneToggle from "../components/CornerstoneToggle"; | ||
|
|
||
| test( "The CornerstoneToggle matches the snapshot", () => { | ||
| const component = renderer.create( | ||
| <CornerstoneToggle onChange={ () => {} } checked={ true } /> | ||
| ); | ||
|
|
||
| let tree = component.toJSON(); | ||
| expect( tree ).toMatchSnapshot(); | ||
| } ); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,115 @@ | ||
| // Jest Snapshot v1, https://goo.gl/fbAQLP | ||
|
|
||
| exports[`The CornerstoneToggle matches the snapshot 1`] = ` | ||
| .c2 { | ||
| background-color: #ccc; | ||
| border-radius: 7px; | ||
| height: 14px; | ||
| width: 30px; | ||
| cursor: pointer; | ||
| margin: 0; | ||
| outline: 0; | ||
| } | ||
|
|
||
| .c2:focus > span { | ||
| box-shadow: inset 0 0 0 1px #fff,0 0 0 1px #5b9dd9,0 0 2px 1px rgba(30,140,190,.8); | ||
| } | ||
|
|
||
| .c3 { | ||
| background-color: #888; | ||
| margin-left: -2px; | ||
| box-shadow: 0 2px 2px 2px rgba(0,0,0,0.1); | ||
| border-radius: 100%; | ||
| height: 20px; | ||
| width: 20px; | ||
| position: absolute; | ||
| margin-top: -3px; | ||
| } | ||
|
|
||
| .c4 { | ||
| padding: 0px 0 0; | ||
| font-size: 14px; | ||
| line-height: 20px; | ||
| width: 30px; | ||
| text-align: center; | ||
| display: inline-block; | ||
| margin: 0; | ||
| font-style: italic; | ||
| } | ||
|
|
||
| .c1 { | ||
| display: -webkit-box; | ||
| display: -webkit-flex; | ||
| display: -ms-flexbox; | ||
| display: flex; | ||
| -webkit-box-pack: justify; | ||
| -webkit-justify-content: space-between; | ||
| -ms-flex-pack: justify; | ||
| justify-content: space-between; | ||
| -webkit-align-items: center; | ||
| -webkit-box-align: center; | ||
| -ms-flex-align: center; | ||
| align-items: center; | ||
| } | ||
|
|
||
| .c0 { | ||
| display: -webkit-box; | ||
| display: -webkit-flex; | ||
| display: -ms-flexbox; | ||
| display: flex; | ||
| -webkit-box-pack: justify; | ||
| -webkit-justify-content: space-between; | ||
| -ms-flex-pack: justify; | ||
| justify-content: space-between; | ||
| -webkit-align-items: center; | ||
| -webkit-box-align: center; | ||
| -ms-flex-align: center; | ||
| align-items: center; | ||
| margin-top: 5px; | ||
| } | ||
|
|
||
| .c0 label { | ||
| margin-right: 10px; | ||
| -webkit-flex-shrink: 0; | ||
| -ms-flex-negative: 0; | ||
| flex-shrink: 0; | ||
| max-width: 75%; | ||
| } | ||
|
|
||
| <div | ||
| className="c0" | ||
| > | ||
| <div> | ||
| <div | ||
| className="c1" | ||
| > | ||
| <label | ||
| htmlFor="Cornerstone Toggle" | ||
| onClick={[Function]} | ||
| > | ||
| Mark this as cornerstone content. | ||
| </label> | ||
| <div | ||
| aria-checked={false} | ||
| aria-label="Mark this post as cornerstone content" | ||
| className="c2" | ||
| id="Cornerstone Toggle" | ||
| onClick={[Function]} | ||
| onKeyDown={[Function]} | ||
| role="checkbox" | ||
| tabIndex="0" | ||
| > | ||
| <span | ||
| className="c3" | ||
| /> | ||
| </div> | ||
| <span | ||
| aria-hidden="true" | ||
| className="c4" | ||
| > | ||
| Off | ||
| </span> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| `; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,137 @@ | ||
| import PropTypes from "prop-types"; | ||
| import React from "react"; | ||
| import styled from "styled-components"; | ||
| import colors from "../../../../style-guide/colors.json"; | ||
| import { __ } from "@wordpress/i18n"; | ||
|
|
||
| const ToggleBar = styled.div` | ||
| background-color: ${ props => props.isEnabled ? "#a5d6a7" : colors.$color_button_border }; | ||
| border-radius: 7px; | ||
| height: 14px; | ||
| width: 30px; | ||
| cursor: pointer; | ||
| margin: 0; | ||
| outline: 0; | ||
| &:focus > span { | ||
| box-shadow: inset 0 0 0 1px ${colors.$color_white}, 0 0 0 1px #5b9dd9, 0 0 2px 1px rgba(30, 140, 190, .8); | ||
|
There was a problem hiding this comment. Unsure as to why the first color is variable, but the other two are not. There was a problem hiding this comment. This applies also to In a way, |
||
| } | ||
| `; | ||
|
|
||
| const ToggleBullet = styled.span` | ||
| background-color: ${ props => props.isEnabled ? colors.$color_green_medium_light : colors.$color_grey_medium_dark }; | ||
| margin-left: ${ props => props.isEnabled ? "12px" : "-2px" }; | ||
| box-shadow: 0 2px 2px 2px rgba(0, 0, 0, 0.1); | ||
|
There was a problem hiding this comment. Same here: this is a hardcoded color. We should use a color from the palette (not necessarily a rgba color) There was a problem hiding this comment. Hi Andrea, you are right. But we'll skip this for now to finish the component. We'll pick this up again during the polishing process. |
||
| border-radius: 100%; | ||
| height: 20px; | ||
| width: 20px; | ||
| position: absolute; | ||
| margin-top: -3px; | ||
| `; | ||
|
|
||
| const ToggleVisualLabel = styled.span` | ||
| padding: 0px 0 0; | ||
|
There was a problem hiding this comment. Seems odd to define Additionally, this means you're deliberately resetting all padding except for the one on the left-hand side. Not sure if this is intended or not. |
||
| font-size: 14px; | ||
| line-height: 20px; | ||
| width: 30px; | ||
| text-align: center; | ||
| display: inline-block; | ||
| margin: 0; | ||
| font-style: italic; | ||
| `; | ||
|
|
||
| const ToggleDiv = styled.div` | ||
| display: flex; | ||
| justify-content: space-between; | ||
| align-items: center; | ||
| `; | ||
|
|
||
| class Toggle extends React.Component { | ||
| /** | ||
| * Sets the toggle object. | ||
| * | ||
| * @param {Object} props The props to use. | ||
| * | ||
| * @returns {void} | ||
| */ | ||
| constructor( props ) { | ||
| super( props ); | ||
|
|
||
| this.onClick = this.props.onToggleDisabled; | ||
|
|
||
| if ( props.disable !== true ) { | ||
| this.onClick = this.setEnabled.bind( this ); | ||
| } | ||
|
|
||
| this.state = { | ||
| isEnabled: this.props.isEnabled, | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the rendered html. | ||
| * | ||
| * @returns {ReactElement} The rendered html. | ||
| */ | ||
| render() { | ||
| return <div> | ||
| <ToggleDiv> | ||
| <label htmlFor={this.props.id} onClick={this.onClick}>{this.props.labelText}</label> | ||
| <ToggleBar isEnabled={ this.isEnabled()} onClick={this.onClick} onKeyDown={this.setEnabled} tabIndex="0" | ||
| role="checkbox" aria-label={this.props.ariaLabel} aria-checked={this.isEnabled()} id={this.props.id} > | ||
| <ToggleBullet isEnabled={this.isEnabled()} /> | ||
|
There was a problem hiding this comment. missing space within braces here and in the lines above |
||
| </ToggleBar> | ||
| <ToggleVisualLabel aria-hidden="true"> | ||
| { this.isEnabled() ? __( "On", "yoast-components" ) : __( "Off", "yoast-components" ) } | ||
|
There was a problem hiding this comment. if this component is going to be reusable, we should be able to pass a different visual label text. There was a problem hiding this comment. Good point. We will make a issue next week to "improve" the toggle: refactor, change how we use colors and add props for the custom on/off label text. |
||
| </ToggleVisualLabel> | ||
| </ToggleDiv> | ||
| </div>; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the current enablement state. | ||
|
There was a problem hiding this comment.
|
||
| * | ||
| * @returns {boolean} The current enablement state. | ||
|
|
||
| */ | ||
| isEnabled() { | ||
| return this.state.isEnabled; | ||
| } | ||
|
|
||
| /** | ||
| * Sets the state to the opposite of the current state. | ||
| * | ||
| * @param {object} evt React SyntheticEvent. | ||
| * @returns {void} | ||
|
There was a problem hiding this comment. Missing whitespace above this line. |
||
| */ | ||
| setEnabled( evt ) { | ||
|
There was a problem hiding this comment. please avoid abbreviations: |
||
| // Makes the toggle actionable with the Space bar key. | ||
| if ( evt.type === "keydown" && evt.which !== 32 ) { | ||
|
There was a problem hiding this comment. I think keycode is safer than which (which is non-standard, though in jQuery is a normalized property in JavaScript it is not) |
||
| return; | ||
| } | ||
|
|
||
| let newState = ! this.isEnabled(); | ||
|
|
||
| this.setState( { | ||
| isEnabled: newState, | ||
| } ); | ||
|
|
||
| this.props.onSetEnabled( newState ); | ||
| } | ||
| } | ||
|
|
||
| Toggle.propTypes = { | ||
| isEnabled: PropTypes.bool, | ||
| ariaLabel: PropTypes.string.isRequired, | ||
| onSetEnabled: PropTypes.func, | ||
| disable: PropTypes.bool, | ||
| onToggleDisabled: PropTypes.func, | ||
| id: PropTypes.string, | ||
| labelText: PropTypes.string, | ||
| }; | ||
|
|
||
| Toggle.defaultProps = { | ||
| isEnabled: false, | ||
| onSetEnabled: () => {}, | ||
| labelText: "", | ||
| }; | ||
|
|
||
| export default Toggle; | ||
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.
the id should be lowercase and without spaces (it's used for HTML attributes). Also, it can't be hardcoded. IDs must be unique in a page, otherwise it's impossible to use more than one toggle.
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.
True, in the toggle it is a prop we can set, but in the cornerstone toggle we keep it hard coded for now because there is only one cornerstone toggle.