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

Show the label for archives block dropdown #30527

Merged
merged 2 commits into from
May 23, 2021
Merged

Show the label for archives block dropdown #30527

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6f1434c
Show the label for archives block dropdown
JustinyAhin Apr 6, 2021
681920b
Remove the label styling in the editor css file
JustinyAhin Apr 6, 2021
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
6 changes: 6 additions & 0 deletions packages/block-library/src/archives/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
ul.wp-block-archives {
padding-left: 2.5em;
}

.wp-block-archives-dropdown {
JustinyAhin marked this conversation as resolved.
Show resolved Hide resolved
label {
display: block;
}
}
2 changes: 1 addition & 1 deletion packages/block-library/src/archives/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ function render_block_core_archives( $attributes ) {

$label = esc_html( $label );

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

Choose a reason for hiding this comment

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

Do we know why this has been added in the first place?

Also, if we decide to remove it, we should remove it from the editor as well. Also, this is the only place where this class is used for Core blocks which means we need to remove the styles defined in common.scss for it as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we know why this has been added in the first place?

When looking at the PR in which this block has been introduced (#7949) I can't find a context about why this class was added in the first place.
I assume it's because they want to hide it (?) but still making it available in the screen reader tree.

Pinging @talldan @mcsf for more details.

Copy link
Contributor

Choose a reason for hiding this comment

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

The original PR matched the widget I believe

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, if we decide to remove it, we should remove it from the editor as well.

It isn't what happens when I remove the class in /block-library/src/archives/index.php? It is removed in the editor and in the frontend.

Copy link
Member Author

Choose a reason for hiding this comment

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

The original PR matched the widget I believe

So it may be to avoid duplicate with the widget title 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, this is the only place where this class is used for Core blocks which means we need to remove the styles defined in common.scss for it as well.

It is also used for the Core Search block

gutenberg/packages/block-library/src/search/index.php

Line 50 in 8be7a8d

'<label for="%s" class="wp-block-search__label screen-reader-text">%s</label>',

Copy link
Contributor

Choose a reason for hiding this comment

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

The original PR matched the widget I believe

It's been a long time, but I assume it was following prior art. I don't know how relevant that label still is.

Copy link
Member Author

Choose a reason for hiding this comment

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

@joedolson , do you think there's an accessibility context for this screen-reader-text class? Thanks :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I also don't recall. It was one of the first PRs I contributed, but I mostly took over from a contributor that wasn't able to finish it.

$block_content = '<label for="' . $dropdown_id . '">' . $title . '</label>
<select id="' . $dropdown_id . '" name="archive-dropdown" onchange="document.location.href=this.options[this.selectedIndex].value;">
<option value="">' . $label . '</option>' . $archives . '</select>';

Expand Down
5 changes: 5 additions & 0 deletions packages/block-library/src/archives/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
.wp-block-archives-dropdown {
label {
display: block;
}
}
1 change: 1 addition & 0 deletions packages/block-library/src/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
display: none;
}

@import "./archives/style.scss";
@import "./audio/style.scss";
@import "./button/style.scss";
@import "./buttons/style.scss";
Expand Down