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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove "block" from "Hide Block Settings" and "Show Block Settings" labels #22955

Merged

Conversation

bartczyz
Copy link
Contributor

@bartczyz bartczyz commented Jun 5, 2020

Description

Update labels from "Hide Block Settings" and "Show Block Settings" to "Hide More Settings" and "Show More Settings".

Closes #22920.

Types of changes

Just labels update, no change in functionality of the component.

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.

@ZebulanStanphill
Copy link
Member

Since the button opens the block inspector, where advanced block settings go, I think the wording should be "Show advanced settings".

@ZebulanStanphill ZebulanStanphill added Needs Copy Review Needs review of user-facing copy (language, phrasing) [Package] Edit Post /packages/edit-post labels Jun 6, 2020
@michelleweber
Copy link

Personally, I'd vote for leaving "Block" in for extra clarity since blocks are still relatively new, but if you're going to be changing it, "Show advanced settings" looks fine.

@bartczyz
Copy link
Contributor Author

bartczyz commented Jun 9, 2020

I would go with either "Show advanced settings" or "Show additional settings".

@ghost
Copy link

ghost commented Jun 22, 2020

Considering that Advanced settings is a group of block settings among other possible ones, it may be better to use a broader term, to refer to all metaboxes that are presented to users.

image

It is good the simplicity and consistency of Show Settings/Hide Settings, as mentioned above and on #22920

Regarding the proposal of Show Additional Settings/Hide Additional Settings, simpler translations are achieved with the synonym Show More Settings/Hide More Settings.

@bartczyz
Copy link
Contributor Author

@marceloaof I really like the "Show More Settings/Hide More Settings" option. Should we go with that?

@ghost
Copy link

ghost commented Jun 22, 2020

Hi @bartczyz If no more considerations are offered in the next hours, it is safe to ask a reviewer to approve "Show More Settings/Hide More Settings". It is a good balance. It serves all mentioned ideas. 👍 Easy-to-read outside english and immediately-understood for screen readers too.

@ZebulanStanphill
Copy link
Member

I'm definitely a fan of "Show more settings". It's simple and gets the point across.

Comment on lines +46 to +47
? __( 'Hide More Settings' )
: __( 'Show More Settings' );
Copy link
Member

Choose a reason for hiding this comment

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

The label should use sentence case to match other controls.

Suggested change
? __( 'Hide More Settings' )
: __( 'Show More Settings' );
? __( 'Hide more settings' )
: __( 'Show more settings' );

Copy link
Contributor Author

@bartczyz bartczyz Jun 23, 2020

Choose a reason for hiding this comment

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

I wanted to be consistent with other labels in that menu:
image

Copy link
Member

Choose a reason for hiding this comment

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

Oh, well in that case I guess it's fine for now. I guess we can change all of those at once in a separate PR.

@mariohamann
Copy link

Do we need this option at all?

  • It is just a duplicate of the existing button in the toolbar.
  • This option changes the UI and therefore – in my opinion – does not really fit to the other actions, which "do something" with or around the selected block.

@ZebulanStanphill
Copy link
Member

@mariohamann The "Show block settings" option was added as a sort of accessibility compromise to make toggling the inspector's visibility easier for keyboard users, who would otherwise have to navigate to the top bar and toggle the sidebar there.

(From an accessibility perspective, the block inspector shouldn't even function as sidebar in the first place. Rather, the block inspector should function as a dialog/popover, and it should be accessed from a button in the block toolbar, not the top toolbar. But that's a separate topic.)

I think this PR needs to be rebased to get tests to run/pass.

@mariohamann
Copy link

mariohamann commented Sep 14, 2020

@ZebulanStanphill Thank you very much for clarification – I assumed it might have something to do with keyboard controls.

Still I'm a bit irrititated: Is the goal of the button to (a) open/close the sidebar or (b) to get access to the sidebar? If the goal is to access the sidebar, might it be more convenient to replace the this option with a "jump link" to the sidebar (always opening the sidebar, even if it's closed)? I tested the current behaviour with keyboard and if the sidebar is open, one has to close and open the sidebar again to get access to it from block level (seeing no focus for a moment).

Furthermore I'm asking myself if there is any possibility to show items only if Gutenberg is being used with keyboard (e. g. as described in this StackOverflow comment), but I'm not sure if this violates other a11y principles and I think this is seperate topic, too. :)

@bartczyz
Copy link
Contributor Author

@ZebulanStanphill I'm on it.

Sorry, was away for a while there. 😉

@bartczyz bartczyz force-pushed the update/hide-block-settings-label branch from 6315ffe to 6e91eed Compare September 14, 2020 17:37
@bartczyz
Copy link
Contributor Author

@ZebulanStanphill rebased and ready to go. 👍

@ZebulanStanphill ZebulanStanphill added [Type] Copy Issues or PRs that need copy editing assistance and removed Needs Copy Review Needs review of user-facing copy (language, phrasing) labels Sep 14, 2020
@ZebulanStanphill ZebulanStanphill merged commit 8d05ce8 into WordPress:master Sep 14, 2020
@github-actions github-actions bot added this to the Gutenberg 9.1 milestone Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Edit Post /packages/edit-post [Type] Copy Issues or PRs that need copy editing assistance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hide Block Settings - should be Hide Settings
4 participants