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 visible labels to BlockPatternPicker pattern selection buttons #19789

Merged
merged 2 commits into from
Mar 18, 2020
Merged

Add visible labels to BlockPatternPicker pattern selection buttons #19789

merged 2 commits into from
Mar 18, 2020

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Jan 21, 2020 •
edited
Loading

Fixes #18737.

Description

The lack of visual labels within the BlockPatternPicker component makes it difficult to understand what the IconButton stands for, i.e. what pattern will be added. Currently, the only way to discover the label is to wait for the tooltip.

Describe the solution you'd like
Either there's a prop to pass visible labels, or visible labels are always present. We'd want to consider if we should use the existing label prop, or if we should have separate label and tooltip props.

I'm testing reusing the existing label of the pattern as the actual label rendered. In that case, we probably would have to improve copies for the columns block.

Here's a quick mockup for what it could look like for the Core Columns block and the CoBlocks Form block:

image

How has this been tested?

Screenshots

It needs more love on the CSS side of things:

Screen Shot 2020-01-21 at 18 49 12

Types of changes

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.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

@gziolo gziolo added [Feature] Block API API that allows to express the block paradigm. [Status] In Progress Tracking issues with work in progress [a11y] Labelling labels Jan 21, 2020
@gziolo gziolo self-assigned this Jan 21, 2020
@gziolo gziolo mentioned this pull request Jan 21, 2020
Closed
@gziolo gziolo requested a review from mapk January 31, 2020 16:49
@gziolo gziolo added the [Type] Enhancement A suggestion for improvement. label Feb 3, 2020
@gziolo gziolo force-pushed the update/block-patterns-picker-label branch 2 times, most recently from 6a89114 to 263cbe9 Compare February 18, 2020 12:40
@gziolo
Copy link
Member Author

gziolo commented Feb 18, 2020 •
edited
Loading

I would appreciate some help from designers. I have trouble doing it myself, mostly because of the fact that in the mock-ups the label is outside of the button. Ideally, both the icon and label should be wrapped with the Button component. It should also account for the longer labels.

@github-actions
Copy link

github-actions bot commented Feb 21, 2020 •
edited
Loading

Size Change: +143 B (0%)

Total Size: 864 kB

Filename Size Change
build/block-editor/index.js 105 kB +31 B (0%)
build/block-editor/style-rtl.css 10.7 kB +35 B (0%)
build/block-editor/style.css 10.7 kB +31 B (0%)
build/block-library/index.js 116 kB +46 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.01 kB 0 B
build/annotations/index.js 3.43 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-library/editor-rtl.css 7.23 kB 0 B
build/block-library/editor.css 7.24 kB 0 B
build/block-library/style-rtl.css 7.38 kB 0 B
build/block-library/style.css 7.39 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.7 kB 0 B
build/components/index.js 191 kB 0 B
build/components/style-rtl.css 15.5 kB 0 B
build/components/style.css 15.5 kB 0 B
build/compose/index.js 5.75 kB 0 B
build/core-data/index.js 10.6 kB 0 B
build/data-controls/index.js 1.03 kB 0 B
build/data/index.js 8.22 kB 0 B
build/date/index.js 5.36 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-post/index.js 91.3 kB 0 B
build/edit-post/style-rtl.css 8.64 kB 0 B
build/edit-post/style.css 8.64 kB 0 B
build/edit-site/index.js 4.64 kB 0 B
build/edit-site/style-rtl.css 2.48 kB 0 B
build/edit-site/style.css 2.48 kB 0 B
build/edit-widgets/index.js 4.44 kB 0 B
build/edit-widgets/style-rtl.css 2.58 kB 0 B
build/edit-widgets/style.css 2.58 kB 0 B
build/editor/editor-styles-rtl.css 381 B 0 B
build/editor/editor-styles.css 382 B 0 B
build/editor/index.js 43.8 kB 0 B
build/editor/style-rtl.css 3.97 kB 0 B
build/editor/style.css 3.96 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 734 B 0 B
build/format-library/index.js 7.09 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.92 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.49 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.68 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.85 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.01 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 780 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.3 kB 0 B
build/server-side-render/index.js 2.55 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.01 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@ItsJonQ
Copy link

ItsJonQ commented Feb 21, 2020

Haiii @gziolo !

After playing around with it for a while, I think the better UI may be to provide shorter labels underneath the buttons (rather than the long labels):

Screen Shot 2020-02-21 at 4 10 49 PM

To ensure screen readers have context, I've added the long labels as aria-label (tested with Mac OS Voiceover)

I think making the label clickable would be ideal. However, I think it may be quite involved to restyle the wrapper button, and simulate a fake button within (to allow for both Button + Label to be clickable).

Alternatively! I think perhaps these may be better if they were input[type="radio"], as that is the UI we actually have :). Granted, we'd still have to do styling to resolve the Button/Label situation.

(Totally can be done later)

Anywhoo! Let me know if my pushed fixes are okay!

@mapk
Copy link
Contributor

mapk commented Mar 6, 2020

Looks good! It'll need a rebase with G2, but worked well regardless.

Thanks for jumping into this @ItsJonQ.

Screen Shot 2020-03-06 at 2 09 29 PM

@gziolo gziolo removed the [Status] In Progress Tracking issues with work in progress label Mar 12, 2020
@gziolo gziolo force-pushed the update/block-patterns-picker-label branch from a1b6b69 to 40feb92 Compare March 12, 2020 16:58
@gziolo gziolo marked this pull request as ready for review March 12, 2020 16:58
@gziolo gziolo force-pushed the update/block-patterns-picker-label branch 3 times, most recently from e1d8bd8 to 5ece310 Compare March 12, 2020 17:51
@gziolo
Copy link
Member Author

gziolo commented Mar 12, 2020

I rebased this branch with master and applied a few minor tweaks:

Screen Shot 2020-03-12 at 18 30 11

@gziolo
Copy link
Member Author

gziolo commented Mar 12, 2020

@ItsJonQ thank you for all your help. I like the approach you took 👍

@gziolo gziolo force-pushed the update/block-patterns-picker-label branch from 5ece310 to 66bc158 Compare March 12, 2020 17:54
gziolo
gziolo commented Mar 12, 2020
View reviewed changes
packages/block-editor/src/components/block-variation-picker/index.js
@@ -46,8 +47,14 @@ function BlockVariationPicker( {
iconSize={ 48 }
onClick={ () => onSelect( variation ) }
className="block-editor-block-variation-picker__variation"
label={ variation.title }
label={ variation.description || variation.title }
Copy link
Member Author

@gziolo gziolo Mar 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

description is optional, so we should fallback to title when not provided.

See:

gutenberg/packages/blocks/src/api/registration.js

Lines 67 to 87 in 7906556

/**
* An object describing a variation defined for the block type.
*
* @typedef {Object} WPBlockVariation
*
* @property {string} name The unique and machine-readable name.
* @property {string} title A human-readable variation title.
* @property {string} [description] A detailed variation description.
* @property {WPIcon} [icon] An icon helping to visualize the variation.
* @property {boolean} [isDefault] Indicates whether the current variation is
* the default one. Defaults to `false`.
* @property {Object} [attributes] Values which override block attributes.
* @property {Array[]} [innerBlocks] Initial configuration of nested blocks.
* @property {Object} [example] Example provides structured data for
* the block preview. You can set to
* `undefined` to disable the preview shown
* for the block type.
* @property {WPBlockVariationScope[]} [scope] The list of scopes where the variation
* is applicable. When not provided, it
* assumes all available scopes.
*/

youknowriad
youknowriad reviewed Mar 13, 2020
View reviewed changes
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

I didn't test but code-wise, this looks good to me. I'll leave the final call for designers.

@gziolo
Copy link
Member Author

gziolo commented Mar 18, 2020

Let's process with this PR as is, we can always iterate later.

@gziolo gziolo merged commit 46c9490 into master Mar 18, 2020
@gziolo gziolo deleted the update/block-patterns-picker-label branch March 18, 2020 08:52
@github-actions github-actions bot added this to the Gutenberg 7.8 milestone Mar 18, 2020
@glendaviesnz glendaviesnz mentioned this pull request Jun 14, 2022
Closed
@afercia afercia mentioned this pull request Nov 14, 2022
Closed
@priethor priethor added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). and removed [a11y] Labelling labels Jul 24, 2023
@t-hamano t-hamano mentioned this pull request Apr 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youknowriad youknowriad youknowriad left review comments

@mapk mapk Awaiting requested review from mapk

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@ellatrix ellatrix Awaiting requested review from ellatrix

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

@nerrad nerrad Awaiting requested review from nerrad

@ntwb ntwb Awaiting requested review from ntwb

@Soean Soean Awaiting requested review from Soean

@talldan talldan Awaiting requested review from talldan

Assignees

@gziolo gziolo

Labels
[Feature] Block API API that allows to express the block paradigm. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Enhancement A suggestion for improvement.
Projects
None yet
Milestone
Gutenberg 7.8
Development

Successfully merging this pull request may close these issues.

Add visible labels to InnerBlocksTemplatePicker pattern selection buttons
5 participants
@gziolo @ItsJonQ @mapk @youknowriad @priethor