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

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

Merged
merged 10 commits into from Aug 18, 2017

Conversation

Projects
None yet
5 participants
@youknowriad
Contributor

youknowriad commented Aug 14, 2017

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?

@youknowriad youknowriad self-assigned this Aug 14, 2017

@youknowriad youknowriad requested review from mtias, jasmussen and aduth Aug 14, 2017

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Aug 14, 2017

Codecov Report

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

Impacted file tree graph

@@            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.

codecov bot commented Aug 14, 2017

Codecov Report

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

Impacted file tree graph

@@            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

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 14, 2017

Contributor

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

screen shot 2017-08-14 at 11 19 20

— 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:

screen shot 2017-08-14 at 11 21 21

@karmatosed any idea for help text?

Contributor

jasmussen commented Aug 14, 2017

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

screen shot 2017-08-14 at 11 19 20

— 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:

screen shot 2017-08-14 at 11 21 21

@karmatosed any idea for help text?

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 14, 2017

Contributor

Perhaps this:

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

jasmussen commented Aug 14, 2017

Perhaps this:

*HTML Anchor*
[ Add anchor label ]
_Anchors lets you link directly to a section on a page._
Show outdated Hide outdated editor/sidebar/block-inspector/generic-controls.js Outdated
Show outdated Hide outdated blocks/inspector-controls/base-control/index.js Outdated
Show outdated Hide outdated blocks/api/serializer.js Outdated
@@ -29,6 +29,8 @@ registerBlockType( 'core/heading', {
className: false,
supportAnchor: true,

This comment has been minimized.

@aduth

aduth Aug 14, 2017

Member

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',
],
@aduth

aduth Aug 14, 2017

Member

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',
],

This comment has been minimized.

@youknowriad

youknowriad Aug 14, 2017

Contributor

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.

@youknowriad

youknowriad Aug 14, 2017

Contributor

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.

This comment has been minimized.

@aduth

aduth Aug 15, 2017

Member

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.

@aduth

aduth Aug 15, 2017

Member

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.

This comment has been minimized.

@youknowriad

youknowriad Aug 15, 2017

Contributor

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?

@youknowriad

youknowriad Aug 15, 2017

Contributor

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?

This comment has been minimized.

@aduth

aduth Aug 15, 2017

Member

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.

@aduth

aduth Aug 15, 2017

Member

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.

This comment has been minimized.

@youknowriad

youknowriad Aug 16, 2017

Contributor

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.

@youknowriad

youknowriad Aug 16, 2017

Contributor

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.

This comment has been minimized.

@BE-Webdesign

BE-Webdesign Aug 16, 2017

Contributor

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.

@BE-Webdesign

BE-Webdesign Aug 16, 2017

Contributor

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.

This comment has been minimized.

@aduth

aduth Aug 16, 2017

Member

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?)

@aduth

aduth Aug 16, 2017

Member

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?)

This comment has been minimized.

@youknowriad

youknowriad Aug 16, 2017

Contributor

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.

@youknowriad

youknowriad Aug 16, 2017

Contributor

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.

This comment has been minimized.

@aduth

aduth Aug 18, 2017

Member

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

aduth Aug 18, 2017

Member

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

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Aug 14, 2017

Member

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

Member

aduth commented Aug 14, 2017

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

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 14, 2017

Contributor

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).

Contributor

youknowriad commented Aug 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Aug 17, 2017

Contributor

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

Contributor

mtias commented Aug 17, 2017

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

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 17, 2017

Contributor

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.

Contributor

jasmussen commented Aug 17, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Aug 17, 2017

Contributor

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

Contributor

mtias commented Aug 17, 2017

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

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 17, 2017

Contributor

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.

Contributor

jasmussen commented Aug 17, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Aug 17, 2017

Contributor

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

Contributor

mtias commented Aug 17, 2017

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

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 17, 2017

Contributor

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.

Contributor

jasmussen commented Aug 17, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Aug 17, 2017

Contributor

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.

Contributor

mtias commented Aug 17, 2017

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

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 17, 2017

Contributor

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

Where should we show this?

Contributor

youknowriad commented Aug 17, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Aug 17, 2017

Contributor

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

Contributor

mtias commented Aug 17, 2017

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

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 17, 2017

Contributor

@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.

Contributor

youknowriad commented Aug 17, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Aug 17, 2017

Contributor

@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.

Contributor

mtias commented Aug 17, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 17, 2017

Contributor

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

Contributor

youknowriad commented Aug 17, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mtias
Contributor

mtias commented Aug 17, 2017

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 17, 2017

Contributor

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

Contributor

youknowriad commented Aug 17, 2017

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

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Aug 17, 2017

Contributor

This looks good to me. 🚢

Contributor

mtias commented Aug 17, 2017

This looks good to me. 🚢

@youknowriad youknowriad merged commit 20d0a13 into master Aug 18, 2017

3 checks passed

codecov/project Absolute coverage decreased by -0.1% but relative coverage increased by +8.71% compared to d585c55
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@youknowriad youknowriad deleted the add/anchor branch Aug 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment