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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add experimental `ResponsiveBlockControl` component #16790

Merged
merged 43 commits into from Nov 1, 2019

Conversation

@getdave
Copy link
Contributor

getdave commented Jul 29, 2019

PLEASE READ THIS BEFORE REVIEWING 馃榾

This PR isn't in any way wired up to Block state. Therefore changing the values in the UI doesn't do anything or persist any settings whatsoever.

This is purely a PR for the UI only to allow multiple controls for different viewports only. It is a sub PR of a wider effort to allow the Group Block to define spacing controls (eg: margin / padding). See below for more details.

Description

Sub PR of Adds basic dimension controls to Group Block.

Adds a standardised <ResponsiveBlockControl> component which provides a UI for Block controls that need to have responsive settings.

Features:

  • Standardised, accessible markup using fieldset and legend
  • Complete control over the rendered controls via render props
  • Render prop to render a default control (non-responsive mode)
  • Toggle to turn responsive controls on/off
  • Render prop to render a Responsive control(s) (responsive mode)

How has this been tested?

I've temporarily hacked and implementation into the core/group Block which utilises <select> inputs and temporary state for demo purposes.

Note that this will be removed before merging.

Screenshots

Default

Screen Shot 2019-07-29 at 11 48 09

Responsive (toggled)

Screen Shot 2019-07-29 at 11 48 18

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@karmatosed

This comment has been minimized.

Copy link
Member

karmatosed commented Aug 2, 2019

Adding a design review here there are a few points come to mind:

image

  • It struck me why no large option. I found that super interesting to experience. I think that's covered in another PR though #16791 maybe?
  • I have to admit it was hard to guess what small/medium would be.
  • I can see people wanting to set the spacing themselves. This is where I think we might be getting into a slippery slope of mammoth configurations but I am noting it.
  • I wonder if using term mobile is future proof? You use your iPad not just mobile.
@karmatosed karmatosed self-requested a review Aug 2, 2019
@getdave getdave requested a review from shaunandrews Aug 2, 2019
@shaunandrews shaunandrews mentioned this pull request Aug 2, 2019
5 of 5 tasks complete
@shaunandrews

This comment has been minimized.

Copy link

shaunandrews commented Aug 2, 2019

Took some time to work up a design.

image

One thing you'll notice is I've reverse the default state of the Switch to be on by default. Switching it off would show the multiple rows.

An alternative ideas is to list out all available options with a Switch for each row. This tends to work great if we plan to have more than 2 options, but otherwise feels a little busy and overwhelming.

image

--

I mentioned some related changes to the placeholder value over in #16791

@shaunandrews

This comment has been minimized.

Copy link

shaunandrews commented Aug 6, 2019

Another consideration here is that its likely we'll have many responsive settings across blocks and even within settings for a single block. For example, consider the need to adjust not only basic spacing (like we have above) but also font-size, or color, or general layout. At some point we may end up with a "mobile" switch for each of these settings. This is going to be problematic.

What we might want to do instead, is have some sort of Switch for the entire editor. Something along the lines of a "mobile mode" or a "tablet mode" that would allow changes to be made to that specific view, without affecting the "desktop mode" display. I realize this is a much larger change than you initial proposed, but I think its the better way of handling the UI.

@getdave

This comment has been minimized.

Copy link
Contributor Author

getdave commented Aug 12, 2019

Took some time to work up a design.

Thanks for much for taking the time here @shaunandrews. It is much appreciated.

I've reverse the default state of the Switch to be on by default. Switching it off would show the multiple rows.

I see why you've done this. Playing devil's advocate here for a second. Does it feel a little odd to be turning something "off" to enable a feature?

...list out all available options with a Switch for each row. This tends to work great if we plan to have more than 2 options, but otherwise feels a little busy and overwhelming.

Nice exploration. I agree this doesn't scale well though.

At some point we may end up with a "mobile" switch for each of these settings. This is going to be problematic.

Yes that could get a little...noisy UI-wise!

What we might want to do instead, is have some sort of Switch for the entire editor. Something along the lines of a "mobile mode" or a "tablet mode" that would allow changes to be made to that specific view, without affecting the "desktop mode" display.

A good idea. I wonder if @youknowriad is aware of any efforts in this area?

I realize this is a much larger change than you initial proposed, but I think its the better way of handling the UI.

I agree although as you've identified it's a lot more complex. @youknowriad @mtias are you able to advise as to whether you feel the direction we're proposing here is suitable or will it be wasted effort in the long run if you have a "mobile mode" in the works?

@mapk

This comment has been minimized.

Copy link
Contributor

mapk commented Aug 14, 2019

provides a UI for Block controls that need to have responsive settings.

This seems very related to the responsive block controls that went through several iterations here: #13363

I think we came super close with this particular design from @kjellr : #13363 (comment)

I understand the UI in #13363 is primarily for columns, but can help inform the design behind spacing too hopefully.

@getdave

This comment has been minimized.

Copy link
Contributor Author

getdave commented Aug 14, 2019

@mapk Thanks very much for your feedback.

I think we came super close with this particular design from @kjellr : #13363 (comment)

@mapk This is what I was building on having touched base with @kjellr earlier. However there have been a few iterations since and @shaunandrews has provided some more input.

Are you able to advise how you feel about the designs @shaunandrews has proposed (cc @kjellr )? I'd like to avoid costly refactors in code because it takes a long time. Ideally, I've love ot get consensus from the design before I write any more code here.

Much appreciated.

@mapk

This comment has been minimized.

Copy link
Contributor

mapk commented Aug 14, 2019

Are you able to advise how you feel about the designs @shaunandrews has proposed

For sure! Sorry to have let this stall. Regarding @shaunandrews' mockups, I definitely prefer the first concept, so I'll comment on that one specifically.

Screen Shot 2019-08-14 at 8 29 54 AM

  1. Looking at this, I first thought the dropdown "Small" was a reference to device size due to the label. To curtail this confusion, we repeated a lot of words in the other mockups for Columns.
  2. I think there will be an a11y problem with switching out the label of the dropdown when toggling on. This will probably not work well with screen-readers.
  3. That second point leads me to think if we move the toggle to the top, and keep the dropdowns below it, that might work better. I'm sure some language would have to be changed to support this, but when toggling the top control, it feels okay to have the bottom controls reflect the change.
@getdave

This comment has been minimized.

Copy link
Contributor Author

getdave commented Sep 12, 2019

@mapk Thanks for this and sorry for the mega delay in responding. I've been away a lot recently.

I first thought the dropdown "Small" was a reference to device size due to the label. To curtail this confusion, we repeated a lot of words in the other mockups for Columns.

I'm not completely clear on what you're recommending we do here to avoid this confusion. I've previously been asked to remove any additional "explainer" text to simplify the UI. I'd love to see/hear more detail here to help ensure I"m on the same page.

I think there will be an a11y problem with switching out the label of the dropdown when toggling on.

I think you're right. The solution should be

  • don't add/remove from the DOM - use show/hide
  • to set the disabled attribute on the element that has been hidden (or even the hidden attribute) to better semantically communicate the change to screenreaders
  • consider which aria roles can communicate a change of focus area

...if we move the toggle to the top, and keep the dropdowns below it, that might work bette

I can do that. I don't think this precludes making the other a11y changes though.

@getdave

This comment has been minimized.

Copy link
Contributor Author

getdave commented Sep 14, 2019

@afercia Regarding this comment, I tried adding fieldset around the fields on a per device basis. However, this leads to a lot of visual and markup duplication of the same term.

See below

Screen Shot 2019-09-15 at 08 39 05

Screen Shot 2019-09-15 at 08 38 46

Notice how the legend and the label are repeating the same word.

Questions

  1. Would you say it's overkill to have a fieldset when there is only a single field (ie: for example, we may not want both margin and padding so it's entirely possible that we'll just have this control for padding).

  2. If no to 1, then how would you recommend resolving the duplication of text/term between label and legend?

  3. When the toggle is activated how would you recommend changing the focus to the new fieldsets and marking the All as no longer "active"? I was previously adding/removing the fields from the DOM on toggle, but I felt that perhaps that wasn't a very accessible approach. I now retain both fieldsets in the DOM and so I thought perhaps toggling a hidden attribute on the parent fieldset might be the best way of indicating the change of state? You may need to try this out to understand what I'm talking about.

Also, are there any other ways I could look to improve this?

@getdave getdave force-pushed the add/responsive-block-controls-component branch from ad2c465 to 7da8329 Sep 14, 2019
getdave added a commit that referenced this pull request Sep 15, 2019
Addresses points raised in #16790
@afercia

This comment has been minimized.

Copy link
Contributor

afercia commented Sep 16, 2019

Quoting from #16791 (comment) my understanding, this control is meant to add padding / margin settings to the group block, with the ability to specify this setting for different types of devices. If this is the case, I'd suggest to use <fieldset> and <legend> elements to group the settings based on devices. Pseudo code:

fieldset
    visible legend: All devices
    DimensionControl: Margin
    DimensionControl: Padding
/fieldset
fieldset
    visible legend: Small screen devices
    DimensionControl: Margin
    DimensionControl: Padding
/fieldset
etc.
@afercia

This comment has been minimized.

Copy link
Contributor

afercia commented Sep 16, 2019

@getdave I see only now the screenshot and questions before my previous comment 馃う鈥嶁檪

Yes, in your example the nested fieldset + legend aren't ideal. That would require to change the HTML structure and the text used for legend / labels.

As general rules:

In the example above I'm not sure the labels for the select are ideal.

  • Label = "Desktop/Tablet/Mobile" doesn't tell what the values within the select are about
  • semantically, that actually means: "Select a small/medium Desktop" which is confusing
  • the selects are meant to set the padding values: they contain padding values therefore they should be labelled "Padding"
  • To clarify what this "padding" relates to, the fieldset legend would give context by clarifying it's about "Desktop"
  • This makes even more sense when there are multiple selects within the same group (e.g. padding, margin)

Would you say it's overkill to have a fieldset when there is only a single field

It really depends on the text used for the labels.
In that case, the visible label should also clarify what the "padding" is about, e.g.:
<label for...>Padding for desktop devices</label>

It is OK to remove all the fieldsets if each label provides all the necessary information.

When the toggle is activated how would you recommend changing the focus to the new fieldsets

Couple of considerations:

  • I'm not sure a switch toggle is the most appropriate control: switch toggles should be used to set a setting that gets immediately saved without the need of submitting. In this case, it justs switches the visibility of a UI section. I'd consider to use a different control in the first place. /Cc @mapk
  • I'd move the toggle control (whatever it is) before all the selects. This way, focus can stay on the toggle and the UI after that changes. Users can explore the content after the toggle as they would do normally. Instead, if the UI that changes is placed before the toggle, they won't have a clue something has changed.
getdave added a commit that referenced this pull request Sep 17, 2019
getdave added a commit that referenced this pull request Sep 17, 2019
Address concerns raised in #16790
@getdave

This comment has been minimized.

Copy link
Contributor Author

getdave commented Sep 17, 2019

@afercia Thanks for your feedback.

Here's a screenshot

Screen Shot 2019-09-17 at 15 50 28

Yes, in your example the nested fieldset + legend aren't ideal. That would require to change the HTML structure and the text used for legend / labels.

I agree. So I've removed the fieldsets but kept it open for developers to utilise these when it is most appropriate. The rendering of the control itself if fully in the control of the developer so they can choose to use fieldsets where that makes sense.

It really depends on the text used for the labels.
In that case, the visible label should also clarify what the "padding" is about, e.g.:

I believe the label per device approach is better in this case. As a result I've amended the label text to include a full description as you advise. However, previously design feedback is that including the full text is too visually "verbose". Therefore I suggest a compromise of providing the full text to screen readers whilst providing a shortened version for sighted users. I think with the context of the "Padding" fieldset legend surrounding all the controls it is clear enough that the device names refer to the setting for "padding" for that device.

I'm not sure a switch toggle is the most appropriate control

I'll leave that to you and the design team. @mapk could you confirm a direction for us here please? Much appreciated.

I'd move the toggle control (whatever it is) before all the selects.

Done 馃憤

@getdave getdave force-pushed the add/responsive-block-controls-component branch from 8214e12 to 195aef5 Oct 30, 2019
Incomplete.
Copy link
Member

jorgefilipecosta left a comment

I left some comments (some are optional and are just my thoughts), but In general, I think the PR is ready 馃憤

function ResponsiveBlockControl( props ) {
const { legend, property, toggleLabel, responsiveControlsActive = false, renderDefaultControl, defaultLabel = __( 'All' ), devices = [ __( 'Desktop' ), __( 'Tablet' ), __( 'Mobile' ) ], renderResponsiveControls } = props;

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

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Oct 30, 2019

Member

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.

getdave added 5 commits Oct 30, 2019
This prop was named based on implementation details. Switching out for more agnostic term.
Addresses #16790 (comment)

Noting that this type of comment seems to be undocumented in Gutenberg鈥
This is a small terminology change which could have a big change on the way developers think about repsonsive settings. We shouldn鈥檛 be thinking about 鈥渄evices鈥 but rather 鈥渧iewports鈥 / screen sizes. We can still present to the user as 鈥渄evices鈥 but the developer should not be tying layout changes to specific devices (or conceptual groups of devices - eg: 鈥淢obile鈥).
Addresses #16790 (comment) by making the state of the responsive mode controlled by the consuming component. This completes the process of making the component fully controlled.
Attempt to relieve some of the overhead associated with having ResponsiveBlockControl be a fully controlled component. By consuming this hook, a developer can wire up a default handler for toggling responsive mode without having to worry about creating their own useState-based hooks.
getdave added 3 commits Nov 1, 2019
This reverts commit 48529f7.
@getdave getdave merged commit afdd6df into master Nov 1, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@getdave getdave deleted the add/responsive-block-controls-component branch Nov 1, 2019
@rodrigodagostino

This comment has been minimized.

Copy link

rodrigodagostino commented Nov 1, 2019

@getdave thanks a lot for your efforts and also for considering my opinion :) I think small screens, medium screens, and large screens is even better than the middle-ground I suggested.
Just wanted to share with you one more little thing: the UI of the spacing panel I've implemented a while ago in my custom blocks.

Opera Snapshot_2019-11-01_152649_localhost
I know it doesn't really fit the current Gutenberg workflow, but it might be a little helpful in the future.

daniloercoli added a commit that referenced this pull request Nov 5, 2019
鈥nmobile/gb-mobile-872-JSApplicationIllegalArgumentException-in-RCTAztecView

* 'master' of https://github.com/WordPress/gutenberg: (56 commits)
  Update: Default gradients. (#18214)
  Fix: setting a preset color on pullquote default style makes the block invalid (#18194)
  Fix default featured image size (#15844)
  Fix postmeta radio regression. (#18183)
  Components: Switch screen-reader-text to VisuallyHidden (#18165)
  [rnmobile] Release 1.16.0 to master (#18261)
  Template Loader: Add theme block template resolution. (#18247)
  Add a README file for storybook directory (#18245)
  Add editor-gradient-presets to get_theme_support (#17841)
  Add "Image Title Attribute" as an editable attribute on the image block (#11070)
  enables horizontal movers in social blocks (#18234)
  [RNMobile] Add mobile Spacer component (#17896)
  Add experimental `ResponsiveBlockControl` component (#16790)
  Fix mover for floats. (#18230)
  Rename Component to WPComponent in docstring (#18226)
  Colors Selector: replace `Aa` text by SVG icon (#18222)
  Removed gif from README (#18200)
  makes the submenu items stacked vertically (#18221)
  Add block navigator to sidebar panel for nav block (#18202)
  Fix: consecutive updates may trigger a blocks reset (#18219)
  ...
@youknowriad youknowriad added this to the Gutenberg 6.9 milestone Nov 11, 2019
CreativeDive added a commit to CreativeDive/gutenberg that referenced this pull request Nov 12, 2019
* Basic component skeleton

* Adds component demo to Group Block

* Make toggle control label dynamic and translatable

* Adds styles

* Automatically render responsive controls based on default control

Continue to allow full customisation of the responsive controls, but by default render the responsive fields using the same markup as the default control. This avoids duplication when consuming the component.

* Wrap each field group in a `fieldset` element

* Invert toggle and language to be on by default.

Addresses points raised in WordPress#16790

* Adds initial tests

* Update tests to resemble how a user interacts with Component

* Add toggle state test

* Update to switch toggle to preceed controls

Addresses concern raised in WordPress#16790

* Update individual control labels to fully describe control for a11y

Address concerns raised in WordPress#16790

* Fixes form ui to improve alignment

Addresses WordPress#16790 (comment)

* Improves i18n of generated control label

Addresses WordPress#16790 (comment).

Note that aria-describedby is the correct type of aria role for this use case https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_aria-describedby_attribute

* Improve a11y by leaving DOM intact between state changes.

* Adds aria description to toggle control for improved a11y context

* Update tests

* Update toggle of responsive controls internal to the component

* Adds test for responsiveControlsActive setting prop

* Add tests to cover custom labels and custom device sets

* Fix to only build default repsonsive controls when necessary

* Adds tests to cover rendering of custom responsive controls

* Update to make label component part of component鈥檚 internal API

Addresses WordPress#16790 (comment).

Also updates tests.

* Adds callback prop to fire when responsive mode state changes

Addresses https://github.com/jorgefilipecosta

* Update to utilise withInstanceId HOC

Addresses WordPress#16790 (comment)

* Removes unused export

This component is now provided by internal API and doesn鈥檛 need exposing.

Addresses WordPress#16790 (comment)

* Mark as 鈥渆xperimental鈥

Addresses WordPress#16790 (comment)

* Remove non exposed component API doc

* Extracts label to be a dedicated component

* Updates to completely switch out DOM when responsive mode is toggled

Please see WordPress#16790 (comment)

* Update to useCallback to avoid expensive DOM re-renders

Addresses WordPress#16790 (comment)

See also WordPress#16791 (comment)

* Updates i18n to provide better descriptions for translators

Addresses WordPress#16790 (comment)

* Updates devices list to contain unique non-translatable slug

This addresses concern that we need to be able to identify a 鈥渟ize鈥 by a value that does not change due to translations.

See WordPress#16790 (comment)

* Ensure renderDefault controls render prop has access to device details

Addresses WordPress#16790 (comment)

* Begin documentation

Incomplete.

* Rename 鈥渓egend鈥 prop to 鈥渢itle鈥

This prop was named based on implementation details. Switching out for more agnostic term.

* Fix incorrect usage of _x and replace with translator comments

Addresses WordPress#16790 (comment)

Noting that this type of comment seems to be undocumented in Gutenberg鈥

* Update internal nomenclature from 鈥渄evice鈥 to 鈥渧iewport鈥

This is a small terminology change which could have a big change on the way developers think about repsonsive settings. We shouldn鈥檛 be thinking about 鈥渄evices鈥 but rather 鈥渧iewports鈥 / screen sizes. We can still present to the user as 鈥渄evices鈥 but the developer should not be tying layout changes to specific devices (or conceptual groups of devices - eg: 鈥淢obile鈥).

* Refactor to make component fully controlled

Addresses WordPress#16790 (comment) by making the state of the responsive mode controlled by the consuming component. This completes the process of making the component fully controlled.

* Adds custom hook useResponsiveBlockControl

Attempt to relieve some of the overhead associated with having ResponsiveBlockControl be a fully controlled component. By consuming this hook, a developer can wire up a default handler for toggling responsive mode without having to worry about creating their own useState-based hooks.

* Revert "Adds custom hook useResponsiveBlockControl"

This reverts commit 48529f7.

* Remove testing implementation in Group Block

* Docs update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.