Block API: Adding the anchor support (id) #2394

youknowriad · 2017-08-14T09:15:41Z

This PR starts the work on #1734

The approach here is to add a new block API property supportAnchor (yeah, another property :(). A block can opt-int to the anchor support by setting supportAnchor: true. This will add an Anchor Text Input to the sidebar (similar to the className behaviour).
The serializer will automatically sets the id attribute to the wrapper node of the block.

Notes

I've used an opt-in approach (enabled on heading only for now), because the serializer can't automatically add an attribute to any random block. Blocks with no wrapper or with a save function returning a string can't declare support for the anchor.

Not done yet:

Automatically generate an id
Check unicity

Thoughs on the approach here?

codecov · 2017-08-14T09:15:43Z

Codecov Report

Merging #2394 into master will decrease coverage by 0.1%.
The diff coverage is 35.29%.

@@            Coverage Diff             @@
##           master    #2394      +/-   ##
==========================================
- Coverage   26.58%   26.47%   -0.11%     
==========================================
  Files         158      158              
  Lines        4857     4911      +54     
  Branches      817      827      +10     
==========================================
+ Hits         1291     1300       +9     
- Misses       3013     3052      +39     
- Partials      553      559       +6

Impacted Files	Coverage Δ
blocks/inspector-controls/toggle-control/index.js	`0% <ø> (ø)`	⬆️
blocks/inspector-controls/select-control/index.js	`0% <ø> (ø)`	⬆️
...locks/inspector-controls/textarea-control/index.js	`0% <ø> (ø)`	⬆️
...locks/inspector-controls/checkbox-control/index.js	`0% <ø> (ø)`	⬆️
blocks/inspector-controls/text-control/index.js	`0% <ø> (ø)`	⬆️
blocks/inspector-controls/base-control/index.js	`100% <ø> (ø)`	⬆️
blocks/inspector-controls/radio-control/index.js	`0% <ø> (ø)`	⬆️
blocks/library/heading/index.js	`23.8% <ø> (ø)`	⬆️
blocks/inspector-controls/range-control/index.js	`100% <ø> (ø)`	⬆️
...ditor/sidebar/block-inspector/advanced-controls.js	`0% <0%> (ø)`
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d585c55...9cbd343. Read the comment docs.

jasmussen · 2017-08-14T09:22:20Z

Love this, I dig the implementation. I think the only thing we need to do is think carefully about this:

— that is, we either need a good label for this, or some help text. Or both.

"HTML Anchor"?

We could perhaps use this pattern for italicized text below:

@karmatosed any idea for help text?

jasmussen · 2017-08-14T11:00:56Z

Perhaps this:

*HTML Anchor*
[ Add anchor label ]
_Anchors lets you link directly to a section on a page._

aduth · 2017-08-14T15:13:51Z

editor/sidebar/block-inspector/generic-controls.js

+/**
+ * Internal constants
+ */
+const ANCHOR_REGEX = /[^\w:.-]/g;


Seems that older specifications had restrictions, but as of HTML5 is not as strict:

https://www.w3.org/TR/2016/REC-html51-20161101/dom.html#the-id-attribute

Or is the hashbang behavior dependent on this?

So we should only avoid spacing characters

aduth · 2017-08-14T15:17:39Z

blocks/inspector-controls/base-control/index.js

 	return (
 		<div className={ classnames( 'blocks-base-control', className ) }>
 			{ label && <label className="blocks-base-control__label" htmlFor={ id }>{ label }</label> }
 			{ children }
+			{ !! help && <p className="blocks-base-control__help">{ help }</p> }


Do we need any accessibility semantics here? aria-describedby?

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_aria-describedby_attribute

Do we need this to be built into the controls themselves, or is it fine if they're just the next sibling in the DOM (i.e. <TextControl /><p>Help Text</p>). Otherwise, we ought to be consistent in how all of the controls accept a help prop.

aduth · 2017-08-14T15:20:06Z

blocks/api/serializer.js

@@ -50,20 +50,28 @@ export function getSaveContent( blockType, attributes ) {
 	}

 	// Adding a generic classname
-	const addClassnameToElement = ( element ) => {
-		if ( ! element || ! isObject( element ) || ! className ) {
+	const addGenericAttributes = ( element ) => {


@mtias I think you'd mentioned these sorts of attributes (class, id) being "advanced". Maybe a more appropriate adjective here?

Yes, I also wanted to render them separately:

Not sure if @jasmussen has other thoughts on how to separate advanced features and make them less pronounced. It may be easier to collapse in place instead of at the footer.

I like this, I think it looks good. I don't think it's a high priority, it seems like an optimization. But I'm a fan of the idea and the look.

Should we rename this method addAdvancedAttributes ?

aduth · 2017-08-14T15:23:04Z

blocks/library/heading/index.js

@@ -29,6 +29,8 @@ registerBlockType( 'core/heading', {

 	className: false,

+	supportAnchor: true,


Trying to think of another way we might scale this. A similar pattern exists in core with post type supports and theme supports. It's a bit different in that case because those supports are always opt-in, which could be represented as an array. If we need some default-true supports, maybe it could still be defined as an object?

supports: { className: false, anchor: true, },

And we could then have a utility method for getting block supports accounting for the default.

Are there other "supports" types we could see adding in the future?

Should className be opt-in? Then we could have:

supports: [ 'className', 'anchor', ],

I personally think the block alignments should be part of these generic attributes. And I don't have a strong preference on how to declare support for these attributes.

I'm not terribly happy with the way className works currently, and I'm curious if it might be improved by changing it to an opt-in. Relating to #2035, it's just not very obvious how this works, particularly when implementing a block with a string return value from save. Changing this to an opt-in at least signals that the block implementer has some understanding of how it works and how they might need to implement the behavior on their own. Otherwise, they can fall into the trap of implementing a string return from save, and users of their block will be presented with a non-functional class name field.

As it relates here, this would have the side-benefit of granting us a consistent pattern for supports. Either it's undeclared and unsupported, or it's declared and therefore supported. Further, this is consistent with how supports work with custom post types and themes.

Agreed, this sounds like a good direction to me. I think @mtias wanted it to be opt-out at first?

Do you think I should update this PR or should we make this consistent separately?

There was another related discussion previously, I think when className was introduced, about the proliferation/scalability of properties on the block API. It may have been @BE-Webdesign that raised it? In any case, it seems wise to build a pattern around such "boolean" features (either block supports it or it doesn't). Even the prefix as it stands here is indicative of scaling trouble: supportAnchor, eventually we might need supportFoo, supportBar.

I don't mean for it to cause trouble for this pull request specifically, but if it's something we feel we ought to address, I think it should either occur here or prior to this pull request being merged.

Mmm, one thing that come to my mind, is that the "className" attribute is not only to declare className support but it serves as a fallback className if it's a string.

I think when className was introduced, about the proliferation/scalability of properties on the block API. It may have been @BE-Webdesign that raised it?

Yeah that was me and it was around useOnce/useMany. That property especially might cause problems as it could have an entirely different value in different contexts.

A supports object sounds like a reasonable way forward. Haven't checked out the branch at all to see how it works.

Mmm, one thing that come to my mind, is that the "className" attribute is not only to declare className support but it serves as a fallback className if it's a string.

What requirements is this feature serving? (Why do we need it?)

It was just an easy way to override the default generated className if a plugin author wanted to do so. But we could remove it for now.

It was just an easy way to override the default generated className if a plugin author wanted to do so. But we could remove it for now.

Can't a block author do this themselves by assigning className: false then assigning a class name of their choosing to the returned element?

aduth · 2017-08-14T15:25:24Z

How do you think we ought to handle blocks which return strings from save?

youknowriad · 2017-08-14T15:35:23Z

How do you think we ought to handle blocks which return strings from save ?

Maybe we don't have to handle them, these are really specific cases where we probably don't need an id. I think the "anchor" this makes sense essentially for heading blocks (all sort of heading blocks).

mtias · 2017-08-17T09:37:55Z

This is looking fine. @youknowriad @jasmussen do you think for headings we could pre-fill with a suggestion based on the heading title?

jasmussen · 2017-08-17T10:06:32Z

do you think for headings we could pre-fill with a suggestion based on the heading title?

I don't know why I feel like this is controversial... but something tells me it is. Perhaps the danger of duplicating IDs if you have the same subheading twice?

On one hand I feel like it adds tremendous value to auto-id all the headings, it's one of my fav. features of github docs. But on the other hand it feels like we should ship this thing as is, and then explore prefilling in a separate PR.

There's also an argumen to be made that we should show a clickable link when you hover over a heading, like on Github, and that if we do this it should probably only happen if you really want that heading to have a anchor. I.e. it may conflict with prefilling.

mtias · 2017-08-17T10:10:15Z

Prefilling wouldn't necessarily add anything automatically, it would only suggest.

jasmussen · 2017-08-17T10:11:57Z

Interesting. How would that work, though? If it's a placeholder, it's replaced when you start typing. The only pattern I can think of is something akin to browser autocomplete, but that seems very difficult to build.

mtias · 2017-08-17T10:13:48Z

It could be like the post format "suggested" thing.

jasmussen · 2017-08-17T10:14:59Z

It could be like the post format "suggested" thing.

Ah, clever. I like it. I still think it shouldn't hold this PR up, unless it's trivially easy to build.

mtias · 2017-08-17T10:16:28Z

Sounds good to leave for later. It may be good to display a post-slug#your-anchor as you are typing the anchor for clarity.

youknowriad · 2017-08-17T10:18:35Z

It may be good to display a post-slug#your-anchor as you are typing the anchor for clarity.

Where should we show this?

mtias · 2017-08-17T10:27:01Z

@youknowriad below the help text, probably. (It could eventually have a "copy link" button.)

…and #

youknowriad · 2017-08-17T11:04:33Z

@mtias Not as easy at seems, we don't know if the URL format (kind of similar to #1285). Can't be done for now.

mtias · 2017-08-17T11:39:59Z

@youknowriad yes, I'd be fine with just returning the ?p-{id} for now. If we do it through a selector, whenever the proper permalink is returned, it should just work.

youknowriad · 2017-08-17T13:09:05Z

I've added the permalink and a button but it probably needs some design :)
I also fixed a bug when parsing a block with an anchor would have shown "external changes" message

mtias · 2017-08-17T13:20:56Z

We can pass the url through https://github.com/WordPress/gutenberg/blob/master/editor/utils/url.js#L36

youknowriad · 2017-08-17T13:24:14Z

@mtias I guess we could move this function to the url package later

mtias · 2017-08-17T16:39:33Z

This looks good to me. 🚢

youknowriad added the [Feature] Block API API that allows to express the block paradigm. label Aug 14, 2017

youknowriad self-assigned this Aug 14, 2017

youknowriad requested review from mtias, jasmussen and aduth August 14, 2017 09:15

youknowriad force-pushed the add/anchor branch from 0f0e158 to 4f9ee64 Compare August 14, 2017 11:13

aduth reviewed Aug 14, 2017

View reviewed changes

youknowriad force-pushed the add/anchor branch from 0e8b695 to 7bf7f7b Compare August 15, 2017 15:25

youknowriad added 5 commits August 17, 2017 11:56

Block API: Adding the anchor support (id)

dfc676b

Blocks: Adding a help text to the anchor input

4e78521

Blocks: Update the anchor regex to allow special chars except spaces …

5474dae

…and #

Inspector Controls: Adding help support to all inspector controls

21fbadd

Blocks: Rename the generic controls (id, class) to advanced controls

c7d550f

youknowriad force-pushed the add/anchor branch from 7bf7f7b to c7d550f Compare August 17, 2017 11:03

youknowriad added 2 commits August 17, 2017 13:43

Advanced Controls: Show the permalink with anchor and copy link button

bb4173d

Advanced controls: Fix Parsing Blocks with anchors

66f9fdf

Advanced controls: Fix the condition for hiding the classname control

0306e4e

youknowriad and others added 2 commits August 17, 2017 14:24

Advanced Controls: Shorten the displayed permalink

39efa0d

Move permalink with anchor to a tooltip.

9cbd343

youknowriad merged commit 20d0a13 into master Aug 18, 2017

youknowriad deleted the add/anchor branch August 18, 2017 09:49

youknowriad mentioned this pull request Aug 18, 2017

Remove save className auto-application #2035

Closed

JeroenSormani mentioned this pull request Apr 15, 2018

Auto fill HTML Anchor (IDs) #6182

Closed

		@@ -29,6 +29,8 @@ registerBlockType( 'core/heading', {

		className: false,

		supportAnchor: true,

Block API: Adding the anchor support (id) #2394

Block API: Adding the anchor support (id) #2394

Conversation

youknowriad commented Aug 14, 2017

codecov bot commented Aug 14, 2017 • edited Loading

Codecov Report

jasmussen commented Aug 14, 2017

jasmussen commented Aug 14, 2017

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

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 Aug 14, 2017

youknowriad commented Aug 14, 2017

mtias commented Aug 17, 2017

jasmussen commented Aug 17, 2017

mtias commented Aug 17, 2017

jasmussen commented Aug 17, 2017

mtias commented Aug 17, 2017

jasmussen commented Aug 17, 2017

mtias commented Aug 17, 2017

youknowriad commented Aug 17, 2017

mtias commented Aug 17, 2017

youknowriad commented Aug 17, 2017

mtias commented Aug 17, 2017

youknowriad commented Aug 17, 2017

mtias commented Aug 17, 2017

youknowriad commented Aug 17, 2017

mtias commented Aug 17, 2017

codecov bot commented Aug 14, 2017 •

edited

Loading