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

Show block anchor attribute in Block Navigation #16898

Closed
wants to merge 4 commits into from

Conversation

pierlon
Copy link
Contributor

@pierlon pierlon commented Aug 4, 2019

Description

Added showing the block anchor ID in the block navigation.

Long anchor IDs is a potential issue that's up for discussion. At the moment I've hidden the overflow.

Fixes #16307.

How has this been tested?

Developed and tested within the provided dockerized Wordpress.

Screenshots

image

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.

@senadir senadir added First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code Needs Design Feedback Needs general design feedback. Needs Testing Needs further testing to be confirmed. labels Aug 4, 2019
Copy link
Contributor

@senadir senadir left a comment

Choose a reason for hiding this comment

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

thank you for your PR @pierlon
I like how you followed the current naming scheme that was introduced in #14420
I tested and it works great.
image

I'm approving this but since it's a design change as well, I will be waiting for another approve from someone in the design team.

@senadir senadir removed the Needs Testing Needs further testing to be confirmed. label Aug 4, 2019
@@ -46,6 +46,7 @@ function BlockNavigationList( {
>
<BlockIcon icon={ blockType.icon } showColors />
{ blockType.title }
{ block.attributes.anchor && <span className="block-editor-block-navigation__item-anchor-name">{ block.attributes.anchor }</span> }
Copy link
Contributor

Choose a reason for hiding this comment

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

What if a block uses the "anchor" attribute for something else? I wonder if we need a dedicated block API to show the "caption" of a block based on its attributes instead?

Thoughts @mtias @gziolo @mcsf

Copy link
Member

Choose a reason for hiding this comment

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

I confirm that this implementation assumes a bit too much about the name of attribute. Enforcing that this attribute can’t be used for anything else might be too cumbersome. We should at least ensure that anchor is one the flag in the supports object defined for the block.

By the way, I know that the mobile part of project experiments with an API which creates a version of the block description which is used for accessibility purposes. It clearly shows that title and description are too generic when we have multiple instances of the same block in one document. We should seek to find a way to make it possible to offer more flexibility on how blocks can be described/announced depending on the context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have refactored the code to check if the block supports the "anchor" feature.

@senadir senadir requested a review from kjellr August 4, 2019 10:17
@karmatosed
Copy link
Member

karmatosed commented Aug 5, 2019

Thanks for this, great to see it move into a PR.

I think having it change color as in original screenshot by @mtias would be great:

image

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.

The "anchor" support is written as a hook, so technically speaking the "block-editor" package has no way to know that this hook exists or not.

That said, I feel like there's a middle ground here between being completely generic (having a block API for this) or being very specific and that's what this PR is achieving.

I think I wouldn't mind trying it like that, but I'd love thoughts from the team @mtias @mcsf

@gziolo
Copy link
Member

gziolo commented Aug 8, 2019

The "anchor" support is written as a hook, so technically speaking the "block-editor" package has no way to know that this hook exists or not.

That said, I feel like there's a middle ground here between being completely generic (having a block API for this) or being very specific and that's what this PR is achieving.

This is exactly what I shared in the 2nd part of my #16898 (comment). I didn't think about the fact that this is something that the block editor should be aware of. In the long run, we should provide some formal API which will allow rendering some sort of previews of the block depending on the context: visual, text-based, screen-reader optimized. At the same time, I wonder how friendly those anchors will be for screen-reader users?

@gziolo gziolo added the Needs Accessibility Feedback Need input from accessibility label Aug 8, 2019
@mcsf
Copy link
Contributor

mcsf commented Aug 8, 2019

This is exactly what I shared in the 2nd part of my #16898 (comment).

An API such as that devised by the mobile team could, in this case, be used directly by the anchor hook implementation, provided that BlockNavigation is extensible.

I didn't think about the fact that this is something that the block editor should be aware of.

This is good to think about, but I wouldn't block this PR based on it. Maybe it serves as some consolation that the anchor hook lives in the block-editor module?

In the long run, we should provide some formal API which will allow rendering some sort of previews of the block depending on the context: visual, text-based, screen-reader optimized. At the same time, I wonder how friendly those anchors will be for screen-reader users?

In my mind there's two ways to look at the problem: enhanced previewing methods, as you suggest, and better systematic extraction of the relevant content in a block.

  • If we consider previewing, how will it solve the current problem in Block Navigation? Would we inject text-based previews for these blocks? If so, how would we reduce noise and focus on things like the anchor? or truncate the rich-text content?

  • If we consider extraction, what can we learn from the mobile project?

@pierlon pierlon force-pushed the add/block-navigation-anchor branch from 409f2e8 to f76b629 Compare August 9, 2019 16:47
@karmatosed karmatosed removed the Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code label Aug 19, 2019
@sarahmonster
Copy link
Member

To evaluate this from a design perspective, it may help to take a few steps back and determine what problem are we trying to solve? by adding this functionality. It's not immediately clear from #16307 what the core user problem that we're solving for is, but there may be some missing context there.

My guess is that the purpose here is one or both of:
a. Providing a way of showing additional context to help users navigate through a potentially complex document tree. This may be better solved by considering #11010 than revealing anchor names, but that would likely depend on how useful anchor names are in context.
b. Allowing users to more easily link to different parts of the page using anchor links. This is most likely a power user move, since not all users will be familiar with the concept and usage of anchor links, and it requires some manual work to accomplish them.

Working backwards, I looked for a way to set an anchor or an ID in the interface itself, but could only find a way of adding classes (without resorting to the HTML editor, that is.) Is the anchor or ID attribute something that users add themselves or are they added programmatically?

If the references are user-defined, they're a potentially useful tool in helping users navigate through a document. This certainly seems like the idea scenario, but I'm struggling to figure out how a user might be able to define an anchor like this... which likely means that most users won't know how to define an anchor, either.

If they're added programmatically, they may not have as much value to the majority of users, who likely won't know what they mean or how to use them. In this case, we run the risk of adding noise to the interface without providing real value.

@hedgefield
Copy link

hedgefield commented Nov 4, 2020

@sarahmonster you can add anchors under Advanced in the sidebar. And when entering a link, you can type the name of that anchor and it will appear in the list. Should be available on most blocks I think.

Screen Shot 2020-11-04 at 12 08 24 Screen Shot 2020-11-04 at 12 09 51

I've only ever used this to mark up headings so I could link directly to that section of a page. I've never thought about using it for adding context (as I've never written anything as complex in an editor that supported anchors) but now that you mention it, that might a use case - though it does feel a bit like an edge case yeah.

If I were to say what I'd use this for, it's to link to specific headings when sharing the URL to my page. So then the most important part for me is to be able to copy that link. The blue color suggested above would indicate that the anchor is clickable, if that then copies the permalink+anchor to my clipboard that would be useful.

As for the location and design, that seems to be in the right place, looks good. I wonder if maybe we could mitigate the length issue by increasing the margins of the items and bit and then placing the anchor below the block name label.

@gziolo gziolo removed the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Jan 5, 2021
@paaljoachim
Copy link
Contributor

What should we do with this PR?

@paaljoachim paaljoachim added [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. and removed Needs Design Feedback Needs general design feedback. labels Feb 8, 2021
Base automatically changed from master to trunk March 1, 2021 15:42
@mtias
Copy link
Member

mtias commented Jul 13, 2021

Closing as this was implemented in #31992.

@mtias mtias closed this Jul 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Accessibility Feedback Need input from accessibility [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show block ID attribute on navigator