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.

Sign up for GitHub

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

Jump to bottom

Add experimental ResponsiveBlockControl component #16790

Merged
merged 43 commits into from
Nov 1, 2019
Merged

Add experimental ResponsiveBlockControl component #16790

Show file tree
Hide file tree
Changes from 34 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
23807cf
Basic component skeleton
getdave Jul 29, 2019
95bfae8
Adds component demo to Group Block
getdave Jul 29, 2019
dd64a53
Make toggle control label dynamic and translatable
getdave Jul 29, 2019
8116762
Adds styles
getdave Jul 29, 2019
4ca0fc5
Automatically render responsive controls based on default control
getdave Sep 14, 2019
fe7c77d
Wrap each field group in a `fieldset` element
getdave Sep 15, 2019
e051305
Invert toggle and language to be on by default.
getdave Sep 15, 2019
9c6e8b2
Adds initial tests
getdave Sep 15, 2019
f2d1bfc
Update tests to resemble how a user interacts with Component
getdave Sep 16, 2019
aa1ec5f
Add toggle state test
getdave Sep 16, 2019
eb5e9ba
Update to switch toggle to preceed controls
getdave Sep 16, 2019
bec9001
Update individual control labels to fully describe control for a11y
getdave Sep 17, 2019
1907640
Fixes form ui to improve alignment
getdave Sep 26, 2019
17eec18
Improves i18n of generated control label
getdave Oct 2, 2019
c9c5e08
Improve a11y by leaving DOM intact between state changes.
getdave Oct 2, 2019
eb9c5f3
Adds aria description to toggle control for improved a11y context
getdave Oct 2, 2019
238d541
Update tests
getdave Oct 4, 2019
1af87b9
Update toggle of responsive controls internal to the component
getdave Oct 4, 2019
51b43a6
Adds test for responsiveControlsActive setting prop
getdave Oct 4, 2019
20a95b4
Add tests to cover custom labels and custom device sets
getdave Oct 4, 2019
a64c2cd
Fix to only build default repsonsive controls when necessary
getdave Oct 4, 2019
9926612
Adds tests to cover rendering of custom responsive controls
getdave Oct 4, 2019
d210cfe
Update to make label component part of component’s internal API
getdave Oct 8, 2019
b81e605
Adds callback prop to fire when responsive mode state changes
getdave Oct 8, 2019
8605087
Update to utilise withInstanceId HOC
getdave Oct 8, 2019
3ac118d
Removes unused export
getdave Oct 28, 2019
045dd49
Mark as “experimental”
getdave Oct 28, 2019
b5bfc06
Remove non exposed component API doc
getdave Oct 28, 2019
5c9841c
Extracts label to be a dedicated component
getdave Oct 28, 2019
b2bea8c
Updates to completely switch out DOM when responsive mode is toggled
getdave Oct 28, 2019
827bb82
Update to useCallback to avoid expensive DOM re-renders
getdave Oct 28, 2019
059b5a5
Updates i18n to provide better descriptions for translators
getdave Oct 28, 2019
ce3be6f
Updates devices list to contain unique non-translatable slug
getdave Oct 30, 2019
195aef5
Ensure renderDefault controls render prop has access to device details
getdave Oct 30, 2019
e821dc9
Begin documentation
getdave Oct 30, 2019
863783c
Rename “legend” prop to “title”
getdave Oct 30, 2019
8ebbd2c
Fix incorrect usage of _x and replace with translator comments
getdave Oct 30, 2019
64e719f
Update internal nomenclature from “device” to “viewport”
getdave Oct 30, 2019
e6aad72
Refactor to make component fully controlled
getdave Nov 1, 2019
48529f7
Adds custom hook useResponsiveBlockControl
getdave Nov 1, 2019
d72e6af
Revert "Adds custom hook useResponsiveBlockControl"
getdave Nov 1, 2019
a46f406
Remove testing implementation in Group Block
getdave Nov 1, 2019
15da1b3
Docs update
getdave Nov 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/block-editor/src/components/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export { default as MediaUpload } from './media-upload';
export { default as MediaUploadCheck } from './media-upload/check';
export { default as PanelColorSettings } from './panel-color-settings';
export { default as PlainText } from './plain-text';
export { default as __experimentalResponsiveBlockControl } from './responsive-block-control';
export {
default as RichText,
RichTextShortcut,
Expand Down
112 changes: 112 additions & 0 deletions packages/block-editor/src/components/responsive-block-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
/**
* External dependencies
*/
import { isFunction } from 'lodash';

/**
* WordPress dependencies
*/
import { __, _x, sprintf } from '@wordpress/i18n';

import { Fragment, useState, useEffect, useCallback } from '@wordpress/element';

import {
ToggleControl,
} from '@wordpress/components';

/**
* Internal dependencies
*/
import ResponsiveBlockControlLabel from './label';

function ResponsiveBlockControl( props ) {
const {
legend,
property,
toggleLabel,
onIsResponsiveModeChange,
renderDefaultControl,
renderResponsiveControls,
responsiveControlsActive = false,
defaultLabel = {
id: 'all',
label: _x( 'All', 'Label. Used to signify a layout property (eg: margin, padding) should apply uniformly to all screensizes.' ),
getdave marked this conversation as resolved.
Show resolved Hide resolved
},
devices = [
{
id: 'small',
label: __( 'Small devices' ),
},
{
id: 'medium',
label: __( 'Medium devices' ),
},
{
id: 'large',
label: __( 'Large devices' ),
},
],
} = props;

const [ isResponsiveMode, setIsResponsiveMode ] = useState( responsiveControlsActive );
Copy link
Member

@jorgefilipecosta jorgefilipecosta Oct 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We are keeping the responsive mode as a local state, but when the flag is toggled the parent needs to know about this state change, otherwise how is the change going to be persisted?

I think the parent should pass something like isResponsiveMode, and an onIsResponsiveMode change and the parent should be responsible for handling and storing changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've also been wrestling with this one.

You'll note until recently this was controlled by the consuming "parent" component. Then in this commit, I moved it to become component/local state.

This was because it felt odd that the consumer needed to provide a means to toggle the UI. I felt the state of the toggle should be internal (thereby avoiding the need for the user to have to know about this implementation detail) but we should expose a callback to notify when it changes and also a means to set the default state.

Note that currently, I've tried to find the best of both worlds. By default you don't need to handle the toggle manually when utilising the component - it is handled internally and "just works" (this is what I'd expect to happen as a user). However, you can optionally set the boolean responsiveControlsActive prop to determine the initial state of the UI (ie: open/closed responsive controls). This allows us to persist an open/closed state between renders and/or editor initialisations. I probably need a decent test case for this scenario.

It's simple enough to roll this back to my original implementation (which is what I think you are recommending) but I am concerned about making this component too hard to utilise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've revised this a little. Let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

This was because it felt odd that the consumer needed to provide a means to toggle the UI.

If the consumer passes isResponsiveMode and onIsResponsiveModeChange the consumer is not passing a means to toggle the UI it is passing if the current mode is responsive and a handler to change it. It is not even aware that the mode is changed by using a toggle.
If responsiveControlsActive changes, the component will not reflect that change, this seems problematic is it makes it impossible to apply a change in the flag from a third party plugin if the component is rendered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be clear

responsiveControlsActive - this sets the initial state of the toggle. This allows the consuming component to set the initial mode to responsive or not.

onIsResponsiveModeChange - this is a callback purely to "notify" the consuming component that the responsive mode has changed to the value provided as an argument (ie: on/off). It doesn't do any setting.

In addition, there is also the internal state value isResponsiveMode. This is used to manage the state of the responsive toggle internally. The consuming component has no means to alter this value except on the initial render where the initial value of the state is set by responsiveControlsActive prop.

My thinking was that it was too much to ask the consuming component to manage the internal toggle state of the UI. I thought that by managing internally it would make the component easier to use. I recognised that the consuming component would need to be able to set the initial toggle state which is why I went for the responsiveControlsActive prop.

That said, recent experience has led me to believe that almost all components that involve user input need to be fully controlled components. I think this is what you were driving at and I'm coming around to that idea, even if it makes the component more labourious to consume.

Copy link
Member

Choose a reason for hiding this comment

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

onIsResponsiveModeChange - this is a callback purely to "notify" the consuming component that the responsive mode has changed to the value provided as an argument (ie: on/off). It doesn't do any setting.

If it does not do any setting, why we have a need to notify it? I think when there is a responsive mode change, the parent must update its state.
If we had a different padding setting per device (one value for small and one for medium), then the user clicks "Use the same padding on all screen sizes." the parent needs to update the values to remove the different settings for small and medium paddings. Otherwise, we start a disconnection between what the UI says, "Use the same padding on all screen sizes." and what is effectively stored.

So It seems like when there is a toggle on the responsive mode, the parent needs to update something probably block attributes. Depending on these attributes, the parent knows if the flag is true or not. If, during the first render, the parent needs to know if responsiveControlsActive is true or false, why can't on future updates the flag the parent passes also be used? That's the reason I think we don't need a local state the parent knows if the toggle is set or not depending on the attributes.

But right now I also not sure on the direction because we don't have storage for these settings yet. Also, I'm not totally inside the problem, so don't treat this suggestion as a blocker we can continue the current approach. When the component is used in practice it will be easier to understand the current path and if needed we can change as the component is experimental.


useEffect( () => {
if ( isFunction( onIsResponsiveModeChange ) ) {
onIsResponsiveModeChange( isResponsiveMode );
getdave marked this conversation as resolved.
Show resolved Hide resolved
}
}, [ isResponsiveMode ] );

const handleToggle = useCallback( ( isChecked ) => {
setIsResponsiveMode( ! isChecked );
} );

if ( ! legend || ! property || ! renderDefaultControl ) {
return null;
}

const toggleControlLabel = toggleLabel || sprintf( _x( 'Use the same %s on all screensizes.', 'Toggle control label. Should the property be the same across all screen sizes or unique per screen size.' ), property );
getdave marked this conversation as resolved.
Show resolved Hide resolved

const toggleHelpText = _x( 'Toggle between using the same value for all screen sizes or using a unique value per screen size.', 'Toogle control help text.' );

const defaultControl = renderDefaultControl( <ResponsiveBlockControlLabel property={ property } device={ defaultLabel } />, defaultLabel );

const defaultResponsiveControls = () => {
return devices.map( ( device ) => (
<Fragment key={ device.id }>
{ renderDefaultControl( <ResponsiveBlockControlLabel property={ property } device={ device } />, device ) }
</Fragment>
) );
};

return (

<fieldset className="block-editor-responsive-block-control">
<legend className="block-editor-responsive-block-control__legend">{ legend }</legend>

<div className="block-editor-responsive-block-control__inner">
<ToggleControl
className="block-editor-responsive-block-control__toggle"
label={ toggleControlLabel }
checked={ ! isResponsiveMode }
onChange={ handleToggle }
help={ toggleHelpText }
/>

getdave marked this conversation as resolved.
Show resolved Hide resolved
{ ! isResponsiveMode && (
<div className="block-editor-responsive-block-control__group block-editor-responsive-block-control__group--default" >
Copy link
Member

Choose a reason for hiding this comment

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

Same note here about BEM convention as at: #17846 (comment)

Also:

  • These classes are only referenced in tests. It's not strictly problematic to do so, but ideally the runtime implementation needn't be conformed specifically to tailor to tests.
  • I think in representing this as some sort of on/off state (it either is or isn't "responsive"), we'd not need to refer to this explicitly as "default" but rather as the absence of "responsive". In other words, I'd suggest using modifier class is-responsive for the other condition, and for this one, simply omitting it.

The second point would actually enable us to simplify this implementation, where currently there is code duplication for block-editor-responsive-block-control__group largely because of how this class is assigned. With the above changes, it could become a unified wrapper of:

<div className={ classnames( 'block-editor-responsive-block-control__group', { 'is-responsive': isResponsive } ) } hidden={ ! isResponsive }>

{ defaultControl }
</div>
) }

{ isResponsiveMode && (
<div className="block-editor-responsive-block-control__group block-editor-responsive-block-control__group--responsive" hidden={ ! isResponsiveMode }>
{ ( renderResponsiveControls ? renderResponsiveControls( devices ) : defaultResponsiveControls() ) }
</div>
) }

</div>
</fieldset>
);
}

export default ResponsiveBlockControl;
21 changes: 21 additions & 0 deletions packages/block-editor/src/components/responsive-block-control/label.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/**
* WordPress dependencies
*/
import { withInstanceId } from '@wordpress/compose';
import { _x, sprintf } from '@wordpress/i18n';
import { Fragment } from '@wordpress/element';

const ResponsiveBlockControlLabel = ( { instanceId, property, device, desc } ) => {
const accessibleLabel = desc || sprintf( _x( 'Controls the %1$s property for %2$s devices.', 'Text labelling a interface as controlling a given layout property (eg: margin) for a given screen size.' ), property, device.label );
return (
<Fragment>
<span aria-describedby={ `rbc-desc-${ instanceId }` }>
{ device.label }
</span>
<span className="screen-reader-text" id={ `rbc-desc-${ instanceId }` }>{ accessibleLabel }</span>
</Fragment>
);
};

export default withInstanceId( ResponsiveBlockControlLabel );

47 changes: 47 additions & 0 deletions packages/block-editor/src/components/responsive-block-control/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
@mixin screen-reader-text() {
border: 0;
clip: rect(1px, 1px, 1px, 1px);
clip-path: inset(50%);
height: 1px;
margin: -1px;
overflow: hidden;
padding: 0;
position: absolute;
width: 1px;
word-wrap: normal !important;
}

.block-editor-responsive-block-control {
margin-bottom: $block-padding*2;
border-bottom: 1px solid $light-gray-600;
padding-bottom: $block-padding;

&:last-child {
padding-bottom: 0;
border-bottom: 0;
}
}

.block-editor-responsive-block-control__legend {
margin: 0;
margin-bottom: 0.6em;
margin-left: -3px;
}

.block-editor-responsive-block-control__label {
font-weight: 600;
margin-bottom: 0.6em;
margin-left: -3px; // visual compensation
}

.block-editor-responsive-block-control__inner {
margin-left: -1px; // visual compensation
}

.block-editor-responsive-block-control__toggle {
margin-left: 1px;
}

.block-editor-responsive-block-control .components-base-control__help {
@include screen-reader-text();
}
3 changes: 3 additions & 0 deletions ...ges/block-editor/src/components/responsive-block-control/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP
Copy link
Contributor

Choose a reason for hiding this comment

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

Any idea how this snapshot ended up minified? Makes it impossible to review changes to the component 😞

Copy link
Member

@aduth aduth Mar 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@tellthemachines It's quite likely that because the test is using a string of HTML for the snapshot, it doesn't know how to make sense of the string as markup of elements to break across lines, vs. say a React element tree. I'd agree though that it sort of defeats the purpose of a snapshot if a future maintainer is unable to assess whether changes in the snapshot are sensible.

I'm not sure exactly what our current configuration will support for those snapshots, but I'd wonder if it could be using the DOM element or if we could adapt somehow for the React element to be used. Otherwise, we may ought to avoid using the snapshot altogether and instead assert directly against specific values we're expecting in the markup.


exports[`Basic rendering should render with required props 1`] = `"<fieldset class=\\"block-editor-responsive-block-control\\"><legend class=\\"block-editor-responsive-block-control__legend\\">Padding</legend><div class=\\"block-editor-responsive-block-control__inner\\"><div class=\\"components-base-control components-toggle-control block-editor-responsive-block-control__toggle\\"><div class=\\"components-base-control__field\\"><span class=\\"components-form-toggle is-checked\\"><input class=\\"components-form-toggle__input\\" id=\\"inspector-toggle-control-0\\" type=\\"checkbox\\" aria-describedby=\\"inspector-toggle-control-0__help\\" checked=\\"\\"><span class=\\"components-form-toggle__track\\"></span><span class=\\"components-form-toggle__thumb\\"></span><svg class=\\"components-form-toggle__on\\" width=\\"2\\" height=\\"6\\" xmlns=\\"http://www.w3.org/2000/svg\\" viewBox=\\"0 0 2 6\\" role=\\"img\\" aria-hidden=\\"true\\" focusable=\\"false\\"><path d=\\"M0 0h2v6H0z\\"></path></svg></span><label for=\\"inspector-toggle-control-0\\" class=\\"components-toggle-control__label\\">Use the same padding on all screensizes.</label></div><p id=\\"inspector-toggle-control-0__help\\" class=\\"components-base-control__help\\">Toggle between using the same value for all screen sizes or using a unique value per screen size.</p></div><div class=\\"block-editor-responsive-block-control__group block-editor-responsive-block-control__group--default\\"><div class=\\"components-base-control\\"><div class=\\"components-base-control__field\\"><label class=\\"components-base-control__label\\" for=\\"inspector-select-control-0\\"><span aria-describedby=\\"rbc-desc-0\\">All</span><span class=\\"screen-reader-text\\" id=\\"rbc-desc-0\\">Controls the padding property for All devices.</span></label><select id=\\"inspector-select-control-0\\" class=\\"components-select-control__input\\"><option value=\\"\\">Please select</option><option value=\\"small\\">Small</option><option value=\\"medium\\">Medium</option><option value=\\"large\\">Large</option></select></div></div><p id=\\"all\\">All is used here for testing purposes to ensure we have access to details about the device.</p></div></div></fieldset>"`;
Loading