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

Site Editor: Rename left sidebar → block library #26517

Merged
merged 1 commit into from
Nov 9, 2020

Conversation

mattwiebe
Copy link
Contributor

@mattwiebe mattwiebe commented Oct 27, 2020

Description

Rename Left Sidebar to Secondary Sidebar to be more semantic and descriptive of function rather than location.

Fixes #25616

How has this been tested?

  • No lint/build errors
  • Edit Site screen still works
  • Edit Post screen still works

Types of changes

Renaming. What could go wrong?

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.

Copy link
Member

@david-szabo97 david-szabo97 left a comment

Choose a reason for hiding this comment

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

Both the post editor and site editor are working! The new name sounds good so take my ✅

On the other hand, I'm not sure if we are going to put anything else in the left sidebar. If we are going to do so, then we need to rename it again to something else that's more general so we can put anything in the left sidebar. So let's see what others think.

@talldan
Copy link
Contributor

talldan commented Oct 28, 2020

Seems like the changes to the Interface component would be breaking changes as well?

There have been proposals to add other types of content to the left sidebar (#22113), so it'd be good not to make Interface's prop too prescriptive.

@gziolo gziolo added the General Interface Parts of the UI which don't fall neatly under other labels. label Oct 28, 2020
@gziolo
Copy link
Member

gziolo commented Oct 28, 2020

@talldan raised a really good point about the specificity of the block library name proposed. I agree that using the word sidebar isn't the best choice we made a long time ago. It quickly became evident that on smaller screens it can never be displayed as a sidebar. In mobile apps, it's handled in yet another way. When searching for a more general name, I recommend looking at WAI-ARIA roles that exist in HTML specification:
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles

@vindl
Copy link
Member

vindl commented Oct 28, 2020

When searching for a more general name, I recommend looking at WAI-ARIA roles that exist in HTML specification:
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles

I browsed through this list but none of the names seem like a good fit. @talldan or @gziolo do you have some alternative proposal in mind?

Copy link
Contributor

@Addison-Stavlo Addison-Stavlo left a comment

Choose a reason for hiding this comment

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

descriptive of function rather than location.

Given this is an interface skeleton, I think it makes sense to be more about location than function. Most of the 'bone names' are location based (header, footer, sidebar, content, etc.). The boneName (for lack of a better term) should be somewhere we can choose to put the BlockLibrary in, and it shouldn't be named with a bias of its functionality.

An analogy to a video game - we would name the item slots of a character something like 'left hand', 'right hand', 'body', etc. where someone might equip a 'sword' item to the 'left hand'. Its a bit odd naming the slot 'sword' and equipping a sword to it, and that kind of feels like what we are doing here with naming the interface slot blockLibrary 🤔

@mattwiebe
Copy link
Contributor Author

We could just call it Sidebar if we're trying to reference location, or even Sheet if we want to be more inspired by Material Design.

@gziolo
Copy link
Member

gziolo commented Oct 29, 2020

It looks like @wordpress/interface is marked as experimental on npm:

https://www.npmjs.com/package/@wordpress/interface

It isn't exposed in WordPress under wp.interface:

Screen Shot 2020-10-29 at 12 07 44

It should be fine to rename both props in the InterfaceSkeleton component:

/* translators: accessibility text for the left sidebar landmark region. */
leftSidebar: __( 'Left sidebar' ),
/* translators: accessibility text for the settings landmark region. */
sidebar: __( 'Settings' ),

The current distinction between left and the regular sidebar is indeed confusing. There is one more thing to consider, how it should be announced for folks that use screen readers for navigation?

When searching for a more general name, I recommend looking at WAI-ARIA roles that exist in HTML specification:
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles

I browsed through this list but none of the names seem like a good fit. @talldan or @gziolo do you have some alternative proposal in mind?

Some options:

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/Complementary_role

The complementary landmark role is used to designate a supporting section that relates to the main content, yet can stand alone when separated. These sections are frequently presented as sidebars or call-out boxes. If possible, use the HTML

element instead.

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/Region_role

The region landmark role is used to identify an area in the document that the author has identified as significant. It is used to provide a generic landmark for people to be able to navigate to easily when none of the other landmark roles are appropriate.

Anyway, I agree that they aren't a perfect match name wise.

@mattwiebe
Copy link
Contributor Author

Since this is actually experimental and renaming should be possible without breaking things, this is what I'm inclined towards:

  1. sidebarprimarySidebar (or mainSidebar)
  2. leftSidebarsecondarySidebar

Because it's one thing to use ARIA roles, it's another to name things. I think that this strikes the right balance. I don't think that ARIA roles make for good actual names.

Also, every area in InterfaceSkeleton is marked as role="region". We could also revisit those, with a few possibilities:

  1. drawer: role="navigation"
  2. secondarySidebar: role="complementary"
  3. body: role="main"
  4. header: role="banner"

@gziolo
Copy link
Member

gziolo commented Nov 2, 2020

@mattwiebe, I agree with your proposal in general. There are some pre-existing conditions that need to be taken into account. WP-Admin already defines some roles like "main" and it is above the code that controls the block editor:

Screen Shot 2020-11-02 at 11 09 27

There should be only one main role in the document. I think this is why the region role was used for the block editor content.

primarySidebar and secondarySidebar sounds better than what we have at the moment. Although it still isn't perfect when using the block editor on the smaller screens where they aren't presented as sidebars:

Screen Shot 2020-11-02 at 11 12 29

Screen Shot 2020-11-02 at 11 12 41

Anyway, if there aren't better ideas we can proceed with those names 👍

Finding the balance between too general and too specific is tricky.

Fixes WordPress#25616
@mattwiebe mattwiebe force-pushed the update/rename-left-sidebar branch from 293185e to 641f7b1 Compare November 6, 2020 20:57
@mattwiebe mattwiebe requested a review from ellatrix as a code owner November 6, 2020 20:57
@mattwiebe
Copy link
Contributor Author

Thanks for all of the helpful feedback and context @gziolo - much appreciated.

I ultimately decided to stay a bit more simplistic here with just renaming leftSidebarsecondarySidebar. I left sidebar alone because its usage is much more deeply ingrained in Gutenberg internals, particularly in core data selectors and dispatch actions.

@gziolo
Copy link
Member

gziolo commented Nov 7, 2020

It looks good to go alone. I like this proposal, it makes the most sense 👍

We should add a soft depreciation for the old name in case some 3rd party projects use the package. I can tackle that part on Monday or share some prior examples.

@gziolo gziolo merged commit 86e5cc1 into WordPress:master Nov 9, 2020
@github-actions github-actions bot added this to the Gutenberg 9.4 milestone Nov 9, 2020
@gziolo
Copy link
Member

gziolo commented Nov 9, 2020

I'll open a follow-up PR with a proposal with deprecations as this branch was created in the forked repository :)

Thanks, @mattwiebe for driving this change 🙇

@gziolo
Copy link
Member

gziolo commented Nov 9, 2020

Hmmm, I'm not quite sure if technically speaking we need this deprecation anymore knowing that we won't publish those changes to npm before WordPress 5.6 is out. Anyway, it won't harm to add it and remove it in a few weeks. See: #26826.

@mattwiebe mattwiebe deleted the update/rename-left-sidebar branch November 11, 2020 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
General Interface Parts of the UI which don't fall neatly under other labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Site Editor: Rename LeftSidebar
6 participants