Fix: Frontend Category Label + Make Category and Archive Labels Visible #11219

Luciaisacomputer · 2018-10-29T20:07:05Z

Description

In #11003 it was mentioned that the Category Dropdown Label should be visible in the Gutenberg Editor as well as the frontend. For #11002 the same was noted for Archives Dropdown for both Gutenberg as well as the frontend.

I've update the two components to render without the screen-reader-text class, while adding a label to the HTML output for the Category dropdown that is rendered on the frontend.

As it currently stands, the spacing of the inputs when used together on the frontend leaves much to be desired. I'm assuming this is controlled by the theme so opted not to write an CSS for that particular problem.

How has this been tested?

I checked the text for each label before and after the changes, made sure my CSS selectors wouldn't cause any visual regressions, ran the linting scripts, and used VoiceOver + Safari to make sure screen readers still interacted correctly.

Screenshots

Gutenberg Editor Category and Archives Dropdowns

Frontend Category and Archives Dropdowns

Additional Screenshots from Code Review Comments + testing in multiple themes

Twenty Fifteen

Twenty Sixteen

Twenty Seventeen

Givingpress

Nifty Lite

Specia

Hestia

ModuAgency

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.

Luciaisacomputer · 2018-10-30T13:52:41Z

If needed, I can cherry pick the front-end label only, and revert the changes in this PR so that we can at least get the labels on the front-end until a full decision is made.

oandregal · 2018-11-15T15:19:18Z

For context, see also #10871

oandregal · 2018-11-15T15:21:03Z

packages/block-library/src/categories/index.php

@@ -31,7 +31,8 @@ function render_block_core_categories( $attributes ) {
 		$wrapper_markup           = '<div class="%1$s">%2$s</div>';
 		$items_markup             = wp_dropdown_categories( $args );
 		$type                     = 'dropdown';
-
+		$title                    = __( 'Categories', 'gutenberg' );
+


It looks like there is a whitespace here that is making the PHP linter unhappy? See https://travis-ci.org/WordPress/gutenberg/jobs/447997322

Thanks, I can make this fix real quick!

oandregal · 2018-11-15T15:23:33Z

packages/block-library/src/archives/index.php

@@ -68,7 +68,7 @@ function render_block_core_archives( $attributes ) {

 		$label = esc_attr( $label );

-		$block_content = '<label class="screen-reader-text" for="' . $dropdown_id . '">' . $title . '</label>


Is this necessary? Per #10871 (comment) it looks like we may want to keep this class.

So I think I got a little eager with this PR, but essentially I felt it was necessary to make the labels visible because it will provide better context to all users. If we can get some design feedback, I can easily revert the removal of these classes. There was some discussion about this in the original issue for this. Maybe @afercia can also speak to why others felt it made sense to make the label visible.

karmatosed · 2018-11-15T21:42:20Z

My concern with this is testing in themes. Have you tested in a range of themes? In the past we've had to scale back what we output and I want to ensure it works across.

Luciaisacomputer · 2018-11-15T23:14:14Z

Thanks for your input @karmatosed. I hadn't considered that. Is there a specific way I can test that? Would it look like me simply switching between multiple WordPress themes both vanilla as well as community related?

Adding the class in is not much of a big deal, I guess it really comes down to what makes the most sense. Personally I think at this point it probably makes sense to add the class back in, and merge the PR with just the front end label. I could then open another ticket to further discuss the design impact visible labels could have.

Thanks again!

karmatosed · 2018-11-19T10:35:33Z

I think at this point it probably makes sense to add the class back in, and merge the PR with just the front end label. I could then open another ticket to further discuss the design impact visible labels could have.

I feel this is best right now. After 5.0 we can do greater testing in a range of themes. I would suggest starting with the default ones. I am very cautious adding styling into this right now.

afercia · 2018-11-19T13:07:28Z

Removing the "Needs Accessibility Feedback" label as a11y feedback is clear: visible label are always preferable.

samikeijonen · 2018-11-19T18:21:31Z

I's also keep visible labels. I tested with default themes and couple of random themes. There are margin bottom issues in all themes which I'd solve in this same PR. That would be margin-bottom: 1.5em; to .wp-block-categories-dropdown and wp-block-archives-dropdown.

Luciaisacomputer · 2018-11-19T18:52:53Z

@samikeijonen Thanks, I take it this means you figured out whatever was preventing my changes from appearing? I can make those changes to the styles for each of those elements.

samikeijonen · 2018-11-19T19:18:53Z

@LukePettway Well, actually I'm now seeing ERROR in ./packages/i18n/build-module/index.js. And can't test at all this branch :) I'm not sure would rebasing from master would help here.

…abels, added category label for frontend

…r archives block and update the margin for it as well

…etter in older themes or when the controls are inline

tofumatt

The styles on the frontend here aren't so great; I expect themes to override them but we should provide nicer defaults I think.

Are the screenshots in the first post here up-to-date? I see a margin-bottom: 1.5em in the styles but the screenshot on the frontend looks like there is no margin and it's really jammed together.

Flagging @jasmussen to have a look and provide some design thoughts.

tofumatt · 2018-11-20T17:55:37Z

packages/block-library/src/archives/editor.scss

@@ -1,3 +1,7 @@
 .block-editor ul.wp-block-archives {
 	padding-left: 2.5em;
 }
+
+.block-editor .wp-block-archives label {


I would rather the label tag get a specific CSS class and use that selector rather than a tag selector; it's convention in the code and makes overlapping/unintended targeting of selectors less likely 😄

Good point, would .wp-block-archives-label be a suitable classname for this? One thing I noticed was that the classes in the archive block aren't really following BEM unlike the ones for the category block.

tofumatt · 2018-11-20T17:57:24Z

packages/block-library/src/archives/style.scss

@@ -0,0 +1,3 @@
+.wp-block-archives-dropdown {
+	margin-bottom: 1.5em;


Not sure where this value is coming from, cc'ing @jasmussen to check on the visuals of this.

@samikeijonen provided the value to me as a suggestion.

It would be good to have a variable for this. But given this is front-end stuff, and we mean to provide some basics, and in this case I think it's okay to use the em unit, as many of the basic elements are born with browser vanilla ems.

In this case, though, I'd take a cue from the figure element, though, and make it 1em:

@jasmussen thanks

In some of the existing block margin bottom in the front-end is 1.5em, that's why I suggested that for consistency.

tofumatt · 2018-11-20T17:57:46Z

packages/block-library/src/categories/editor.scss

@@ -5,3 +5,7 @@
 		margin-top: 6px;
 	}
 }
+
+.block-editor .wp-block-categories label {


Same here re: preferring a class selector over a tag.

tofumatt · 2018-11-20T17:58:08Z

packages/block-library/src/style.scss

@@ -3,6 +3,7 @@
 @import "./block/indicator/style.scss";
 @import "./button/style.scss";
 @import "./categories/style.scss";
+@import "./archives/style.scss";


This should be alphabetised 😄

Whoops! I'll fix this :)

Also in response to your question about the screenshots, I guess the confusion is from me not updating the first two screen shots. The ones that follow are up to date, I left those in there just in case someone wanted to see what it looked like originally.

aduth · 2019-01-04T13:50:00Z

There are failing PHPCS style violations here:

FILE: /app/packages/block-library/src/categories/index.php
----------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
----------------------------------------------------------------------
 35 | ERROR   | [x] Spaces must be used for mid-line alignment; tabs
    |         |     are not allowed
    |         |     (WordPress.WhiteSpace.DisallowInlineTabs.NonIndentTabsUsed)
 36 | WARNING | [x] Equals sign not aligned with surrounding
    |         |     assignments; expected 13 spaces but found 1
    |         |     space
    |         |     (Generic.Formatting.MultipleStatementAlignment.NotSameWarning)
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

aduth

An issue we now have with this is that, as it stands, revisions to block PHP in Gutenberg don't take precedence over WordPress 5.0 implementations. I don't think that needs to be resolved as part of this pull request, but it must be resolved for the changes here to have any impact to those using the plugin.

aduth · 2019-01-07T15:30:28Z

packages/block-library/src/categories/index.php

@@ -31,6 +31,9 @@ function render_block_core_categories( $attributes ) {
 		$wrapper_markup           = '<div class="%1$s">%2$s</div>';
 		$items_markup             = wp_dropdown_categories( $args );
 		$type                     = 'dropdown';
+		$title                    = __( 'Categories' );
+		$label_class              = 'wp-block-categories-label';
+		$items_markup             = '<label class="' . $label_class . '" for="' . $id . '">' . $title . '</label> ' . $items_markup;


I believe $title ought to be escaped here, perhaps via esc_html__

https://developer.wordpress.org/reference/functions/esc_html__/

aduth

As far as the code here, this all looks good 👍

This needs some process consideration since, as noted previously, this code doesn't currently do anything.

I'm fine to move forward with merging this and working separately to change the way by which we extend core blocks (#11015). We'll still need to bring these changes upstream to core, though. I think a Trac ticket may be necessary, including an equivalent patch to as done here. The fact that it's been reviewed here already should hopefully expedite a review for commit.

youknowriad · 2019-01-21T15:30:56Z

At the moment, this PR don't require a trac ticket as Core syncs those files automatically from the packages.

aduth · 2019-01-21T15:33:46Z

At the moment, this PR don't require a trac ticket as Core syncs those files automatically from the packages.

How does this happen? I suppose it has an impact on how #11015 is approached then too, rather than extending the core blocks, needing to substitute their implementations entirely.

youknowriad · 2019-01-21T15:35:11Z

@aduth I think the files are just copied using a grunt task.

Sign-off for flagged design feedback: #11219 (comment)

aduth

Final testing revealed a few more issues.

aduth · 2019-01-21T16:58:22Z

packages/block-library/src/archives/editor.scss

@@ -1,3 +1,7 @@
 .block-editor ul.wp-block-archives {
 	padding-left: 2.5em;
 }
+
+.block-editor .wp-block-archives__label {
+	display: block;


Should this not also be display: block; on the front-end?

This note applies to the categories block as well.

At the time I think I wanted these to be as least opinionated as possible in themes but I can make these changes to render the controls in the same manner on the front end.

So why is it part of the editor styles in the first place?

aduth · 2019-01-21T16:59:31Z

packages/block-library/src/archives/style.scss

@@ -0,0 +1,3 @@
+.wp-block-archives-dropdown {


This class is never actually assigned to the select on the front-end, so it's not applied.

This note applies to the categories block as well.

Hmm I thought these were being added with the below code:

$class = "wp-block-categories wp-block-categories-{$type}";

I'll take a look and see what I find out

It looks like it is being applied but it has a low specificity compared to a bunch of other selectors.

Luciaisacomputer · 2019-08-27T14:04:36Z

@youknowriad do you remember what the issue was with this PR? I think I remember something outside of GH coming up and that's why we never merged it. If this is still a fix we want I can do the necessary work to bring it across the finish line.

youknowriad · 2019-09-14T13:20:58Z

To be honest, I don't remember that much. I know one of the issues was inconsistency between similar blocks. Other than that, I don't remember. Maybe we can try to refresh the PR and get it reviewed once again? Do you think it's ready?

youknowriad · 2020-03-18T10:51:26Z

Looks like this PR is still needed. Do you all have some time to refresh it and rebase it and try to land it?
Are these comments the last blockers? #11219 (review)

paaljoachim · 2020-05-27T08:09:51Z

Hello everyone.
Can we get a status update to where this PR is at?
Thanks.

mcsf · 2021-02-18T12:29:59Z

Per the latest comments from early 2020, closing this as stale. Feel free to reopen with iterations.

Luciaisacomputer mentioned this pull request Oct 30, 2018

Categories block misses a label when set to "Display as Dropdown" #10871

Closed

youknowriad added Needs Design Feedback Needs general design feedback. Needs Accessibility Feedback Need input from accessibility labels Oct 30, 2018

oandregal reviewed Nov 15, 2018

View reviewed changes

mtias added this to the 4.5 milestone Nov 15, 2018

mtias added [Block] Archives Affects the Archives Block [Block] Categories Affects the Categories Block labels Nov 15, 2018

gziolo requested a review from tofumatt November 19, 2018 09:07

afercia removed the Needs Accessibility Feedback Need input from accessibility label Nov 19, 2018

Luke Pettway added 5 commits November 19, 2018 14:53

removed screen reader text class, added styling for eidtor dropdown l…

7ec451e

…abels, added category label for frontend

remove extra whitespace

bef9132

remove whitespace

7a92703

update margin for categories dropdown block, and create stylesheet fo…

73bf117

…r archives block and update the margin for it as well

added space in category dropdown concatenation so visually it looks b…

13d2c62

…etter in older themes or when the controls are inline

mtias modified the milestones: 4.5, 4.6 Nov 19, 2018

tofumatt requested a review from jasmussen November 20, 2018 18:00

tofumatt previously requested changes Nov 20, 2018

View reviewed changes

Luke Pettway added 2 commits November 20, 2018 13:51

change order of imports to be alphabetically correct

f8aaed9

update margin to be 1em instead of 1.5em for consistency across the UI

1fbf486

move label string concatenation inside of check for dropdown display

499096f

fix linting issues with equal sign spacing

aabce13

aduth reviewed Jan 7, 2019

View reviewed changes

youknowriad modified the milestones: 4.9 (Gutenberg), 5.0 (Gutenberg) Jan 21, 2019

escape html for title

b85f3e7

aduth approved these changes Jan 21, 2019

View reviewed changes

aduth requested changes Jan 21, 2019

View reviewed changes

youknowriad modified the milestones: 5.0 (Gutenberg), 5.1 (Gutenberg) Feb 4, 2019

gziolo modified the milestones: 5.1 (Gutenberg), 5.2 (Gutenberg) Feb 17, 2019

gziolo modified the milestones: 5.2 (Gutenberg), 5.3 (Gutenberg) Mar 4, 2019

youknowriad removed this from the 5.3 (Gutenberg) milestone Mar 18, 2019

Luciaisacomputer requested review from ajitbohra, Soean and talldan as code owners October 1, 2020 09:41

mcsf closed this Feb 18, 2021

JustinyAhin mentioned this pull request Apr 6, 2021

Show the label for archives block dropdown #30527

Merged

4 tasks

		@@ -68,7 +68,7 @@ function render_block_core_archives( $attributes ) {

		$label = esc_attr( $label );

		$block_content = '<label class="screen-reader-text" for="' . $dropdown_id . '">' . $title . '</label>

		@@ -0,0 +1,3 @@
		.wp-block-archives-dropdown {
		margin-bottom: 1.5em;

Fix: Frontend Category Label + Make Category and Archive Labels Visible #11219

Fix: Frontend Category Label + Make Category and Archive Labels Visible #11219

Conversation

Luciaisacomputer commented Oct 29, 2018 • edited by gziolo Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

Luciaisacomputer commented Oct 30, 2018

oandregal commented Nov 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oandregal Nov 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karmatosed commented Nov 15, 2018

Luciaisacomputer commented Nov 15, 2018

karmatosed commented Nov 19, 2018

afercia commented Nov 19, 2018

samikeijonen commented Nov 19, 2018 • edited Loading

Luciaisacomputer commented Nov 19, 2018

samikeijonen commented Nov 19, 2018

tofumatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luciaisacomputer Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Jan 4, 2019

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth left a comment

Choose a reason for hiding this comment

youknowriad commented Jan 21, 2019

aduth commented Jan 21, 2019

youknowriad commented Jan 21, 2019 • edited Loading

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luciaisacomputer Jan 21, 2019 • edited Loading

Choose a reason for hiding this comment

Luciaisacomputer commented Aug 27, 2019

youknowriad commented Sep 14, 2019 • edited Loading

youknowriad commented Mar 18, 2020

paaljoachim commented May 27, 2020

mcsf commented Feb 18, 2021

Luciaisacomputer commented Oct 29, 2018 •

edited by gziolo

Loading

oandregal Nov 15, 2018 •

edited

Loading

samikeijonen commented Nov 19, 2018 •

edited

Loading

Luciaisacomputer Nov 20, 2018 •

edited

Loading

youknowriad commented Jan 21, 2019 •

edited

Loading

Luciaisacomputer Jan 21, 2019 •

edited

Loading

youknowriad commented Sep 14, 2019 •

edited

Loading