[WIP] Add footnotes support to Gutenberg #6549

Aljullu · 2018-05-03T00:01:35Z

Description

This is a WIP Pull Request with an implementation of footnotes for Gutenberg. Issue #1890.

Screenshots

Types of changes

It is a new feature which adds footnotes support to the Gutenberg editor.

User visible changes

It adds a button to the toolbar. When clicked, it inserts a footnote (a superscript number) in the content and the footnotes block displays a RichText input area to enter the text for that footnote.

(the icon is not definitive)

Internal changes

Footnotes are stored in a new attribute associated to the block called blockFootnotes. For now, only the Paragraph block has footnotes implemented.
A footnotes block is inserted to the post when the first footnotes is added and removed when the last footnote is removed.
When blocks are modified, inserted, removed, etc. the footnotes block is updated with the new list of footnotes.

How has this been tested?

The relevant parts of the code are covered by tests.

In order to test it manually: create or edit a post and focus on a paragraph, a new button should appear in the toolbar allowing you to add footnotes. Once clicked, a footnotes block should appear with a 1. and an empty space to write the text for the footnote.

Checklist

Add button to the toolbar that adds a footnote in the code.
Create footnotes block which allows writing footnotes.
Use auto-incrementing numbers instead of asterisks for footnotes.
When published, footnote numbers must link to the associated footnote in the footer.
Make the footnotes block only exist when there are footnotes in the content.
Rename data-footnote-id to data-footnote-wp-id.
Footnotes attributes should be extracted from the markup instead of being saved in the comment JSON. This applies both to the footnotes block and to the blocks containing footnotes.
~~Save the footnotes texts as plain HTML~~: given that we are no longer saving the footnotes texts attribute in the JSON comments, there is no need to convert it to plain HTML. Indeed, all other blocks using RichText save text as an array of nodes.

Open bugs

Duplicating blocks with footnotes makes two footnotes to have the same uid: a proposed solution would consist on making the footnotes id to be <block-uid>-<footnote-uid>. Inside the block, they are identified by the footnote id, but in the footnotes block and when saving the post, they are identified with the combination of block uid + footnote id.
Use a specific icon for the footnotes button.

Next steps

This is a list of 'big' tasks that will require some time to implement/fix:

Allow adding footnotes in blocks other than paragraphs: this shouldn't be a hard task but we delayed it until being sure about the convenience of the current implementation.
Improve footnotes interaction with TinyMCE editor: currently deleting/copying/pasting footnotes is hard because they use three nested elements (sup > a > span).
Implement special behavior for the footnotes block: that includes making it non-removable and blocking HTML edition on it.
Performance: the biggest performance problem is that the footnotes block is re-rendered on every change in other blocks' attributes.
That happens because the footnotesOrder prop is an array, so when performing the shallow equal comparison on componentWillReceiveProps (https://github.com/WordPress/gutenberg/blob/master/data/index.js#L293) it thinks it's a different prop and sets shouldComponentUpdate to true even if the array contains the same footnotes. There are several possible solutions to this problem. I will be investigating which is the best one in the following days.

nb · 2018-05-09T20:34:57Z

Thanks for workin on this, @Aljullu! Few high-level questions:

What are the tradeoffs of this being implemented per-block (paragraph like it's now) or per-component (e.g. for all RichText ones)?

What's the expected structure of the texts attribute? Now I see things like "6d8d7295-4065-4205-b5f1-70df6768388c":["adjkash",{"type":"br","key":"_domReact41","ref":null,"props":{},"_owner":null,"_store":{}},"","asdsa",{"type":"br","key":"_domReact44"….

What other alternatives were considered for representing the list of footnotes? I can see several drawbacks in using a block for that (many operations need to be disabled, might not be future-proof, makes it complicated for other block-level editors to implement the feature), but I am not sure there is a better option.

Do we need to touch the parser? Can't we do our job at a higher-level of abstraction, so that we don't pollute the parser code with block-specific logic?

ZebulanStanphill · 2018-05-10T00:09:30Z

This looks pretty neat!

I think the footnotes of a document should be stored in the meta of post, and not directly in a block. The block would merely provide the visual display of the footnotes, as well as provide the place where you would edit them. (Though a panel in the Document sidebar or perhaps a separate sidebar using the plugin sidebar API to view/edit the footnotes could also be added.)

Also, if this is implemented, it should definitely be implemented for at least everything using the RichText component; I can not think of any cases where you would not want the ability to use footnotes in a block using the RichText component.

Aljullu · 2018-05-10T18:43:36Z

Thank you for your comments, I will try to reply all of them:

@nb

What are the tradeoffs of this being implemented per-block (paragraph like it's now) or per-component (e.g. for all RichText ones)?

I'm not sure to understand you here. You mean the parsing should be implemented in RichText or that RichText must be the responsible of updating the list of footnotes in the state?

What's the expected structure of the texts attribute?

The texts attribute is used to store the texts attached to each footnote. So every footnote uid is a key and the text associated is the value.

What other alternatives were considered for representing the list of footnotes? I can see several drawbacks in using a block for that (many operations need to be disabled, might not be future-proof, makes it complicated for other block-level editors to implement the feature), but I am not sure there is a better option.

The main reason I created a "footnotes block" was to be able to reorder it and add other blocks below it. I can see several use cases when it might be interesting to add content after the footnotes (for example, Wikipedia articles usually show a "See also" and "External links" section after the "References", something similar might be desirable in Gutenberg, I think).

However, it's true implementing it as a block can be problematic. Also, making the block auto-appear and disappear depending on whether the other blocks contain footnotes or not is not straight-forward and means we have to be aware of every block change that might modify the footnotes, like split or remove...

Do we need to touch the parser? Can't we do our job at a higher-level of abstraction, so that we don't pollute the parser code with block-specific logic?

Sure! The function added to parser.js can be easily moved somewhere else. Depending on the way we end up implementing it, it might go to a utils.js file or inside the RichText component if needed. I initially placed it in parser.js because semantically it "parses" the content, but there is no other reason it must be there.

@SuperGeniusZeb

I think the footnotes of a document should be stored in the meta of post, and not directly in a block.

Storing them in the meta of the post sounds good to me too, but at the same time I see some disadvantages. Storing them in the block attributes, the footnotes are logically linked to the block, so we get some things for free: when removing a block, for example, the footnotes are removed with it. Also we get the footnotes ordered for free.

Storing them in the meta of the post means we have to update the list when some actions are performad like remove, reorder and split blocks.

The block would merely provide the visual display of the footnotes, as well as provide the place where you would edit them. (Though a panel in the Document sidebar or perhaps a separate sidebar using the plugin sidebar API to view/edit the footnotes could also be added.)

In that case, I think we should still use RichText for writing the footnotes texts. I imagine people using footnotes might want to use links, italics and possibly break lines. I don't know whether that's possible in the current sidebar text fields and whether it would be easy to implement it.

Also, if this is implemented, it should definitely be implemented for at least everything using the RichText component; I can not think of any cases where you would not want the ability to use footnotes in a block using the RichText component.

Yes, it was my goal to implement it in every block that has a RichText. But maybe there is an easier way implementing it directly in RichText so we don't have to update all other blocks?

nb · 2018-05-17T08:01:58Z

I'm not sure to understand you here. You mean the parsing should be implemented in RichText or that RichText must be the responsible of updating the list of footnotes in the state?

I think you answered this in the end of your comment saying: “Yes, it was my goal to implement it in every block that has a RichText. But maybe there is an easier way implementing it directly in RichText so we don't have to update all other blocks?” Ideally we should be able to add footnotes in all RichText components. It’s not a must-have, but it would be cool to have a way for way for developers to add footnotes support for other blocks/components. For example if I am writing a contact form plugin, I may want to allow users to add footnotes to form fields.

nb · 2018-05-17T08:06:27Z

What's the expected structure of the texts attribute?

The texts attribute is used to store the texts attached to each footnote. So every footnote uid is a key and the text associated is the value.

In my case (#6549 (comment)) I saw a lot more than text in the value. Were the React Element objects also supposed to be part of texts?

nb · 2018-05-17T08:32:40Z

However, it's true implementing it as a block can be problematic. Also, making the block auto-appear and disappear depending on whether the other blocks contain footnotes or not is not straight-forward and means we have to be aware of every block change that might modify the footnotes, like split or remove...

I totally agree with your assessment and I wonder what else could we explore here. Some random (and probably bad ideas):

Instead of trying to cut features from blocks, maybe we change how blocks work and switch to a whitelist of available actions per block type.
Even more special footnotes block type that is virtually immutable with the exception of moving it and deleting it. This would mean that editing of footnotes text is moved to a special UI closer to where we add them.

nb · 2018-05-17T08:53:14Z

core-blocks/paragraph/index.js

@@ -258,7 +258,7 @@ class ParagraphBlock extends Component {
 								setAttributes( {
 									content: before,
 									blockFootnotes: parseFootnotesFromContent( before ),
-								} );
+								}, false );


Reading this code, it's hard for me to understand the purpose of the second argument, it’s just a lonely false.

Also, why wouldn't we want to update the blocks visibility in this specific case?

100% agree on this, I don't like passing booleans as parameters either. I refactored a bit this code to optimize it and make it easier to understand in 089dd9d.

Adds a button to the toolbar to add footnotes. When clicked, a SUP tag is added the paragraph content. For now, the button is only activated in paragraphs blocks.

Everytime the block is modified, it parses its code and stores the footnotes in the block object.

…attributes update On editor setup create the footnotes block if it doesn't exist and when any block attributes are updated, update the footnotes block with the new parsed footnotes. This commit depends on a followup that creates the footnotes block.

Create Footnotes block which renders the list of footnotes texts when saved and allows editing them in the post editor.

Make footnotes be numbers instead of asterisks and link to the relevant footnote.

In the case a block is inserted or removed from the list of blocks, footnotes might change. Because of that, we must update the list of footnotes in those cases.

Until now, the footnotes block was always rendering OL tags even if there were no footnotes.

…ith the other functions

…rty object That allows us to parse the block texts when updating its attributes intead of doing it when the new attributes are handled by the reducer. This will make it easier to expand the footnotes feature to blocks other than the paragraph block.

…hen footnotes change This allows us to clean a lot of code from the reducer.

… required This allows us to create the block only if footnotes are present, before we were attaching the footnotes block on editor setup.

…ordpress/blocks

Otherwise, getFootnotes wasn't returning the footnotes of inner blocks.

Only check if footnotes block must be added or removed when the footnotes of a block change. Until now, the check was made everytime a block changed its attributes or was split.

…ntly added tests A better solution will be needed in order to ignore the key during tests so adding more tests doesn't break the previous ones.

Code continues working as usual but it has been optimized, made simpler and the dependency to getFlattenedBlocks has been removed, so it no longer has to be stored in the utils.

Aljullu · 2018-05-21T19:17:45Z

core-blocks/test/fixtures/core__pullquote__multi-paragraph.json

@@ -15,7 +15,7 @@
                                "Paragraph ",
                                {
                                    "type": "strong",
-                                    "key": "_domReact71",
+                                    "key": "_domReact73",


I'm not sure why, but my changes made this test to fail even though they don't seem to be related. I changed the key for now so the tests pass again and will investigate it further in the following days.

I think you can drop this information from the tests, along with the ref etc. Have a look at some of the others tests, some of which should not include these properties.

I created an independent PR to deal with this: #7448.

Aljullu · 2018-05-21T19:25:42Z

I added some more commits which address some of the topics we discussed above. They also improve performance, fix some bugs and are a general clean up of the code.

@nb feel free to take another look when you have a moment!

There was a bug that when reordering blocks with footnotes, those were not reordered when saving the post.

nb · 2018-05-23T07:18:16Z

core-blocks/footnotes/editor.js

+		};
+	}
+
+	componentWillReceiveProps( nextProps ) {


This lifecycle method will be deprecated soon.

Right! Will replace it.

nb · 2018-05-23T07:20:28Z

core-blocks/footnotes/editor.js

+		const { footnotesOrder } = this.props;
+		const nextFootnotesOrder = nextProps.footnotesOrder;
+
+		if ( ! isEqual( footnotesOrder, nextFootnotesOrder ) ) {


Do you think we can avoid the deep comparison here?

I can't think of any way to avoid this comparison for now.

What we could easily do is to store these two arrays as an array of strings instead of an array of objects (from [ { id: 'abcd' }, ... ] to [ 'abcd', ... ]). That will make the comparison a little bit faster.

I refactored the footnotes block so this comparison is no longer necessary (11794cc).

nb · 2018-05-23T07:28:58Z

core-blocks/footnotes/editor.js

+}
+
+export default withSelect( ( select ) => ( {
+	footnotesOrder: select( 'core/editor' ).getFootnotes(),


I think we can improve this name a bit – it doesn't only include the order, but also the footnotes. How about orderedFootnotes?

Sounds good.

I ended up renaming it to oderedFootnoteUids because it only includes the footnote uids (not the footnotes text), so I wanted to distinguish it from footnotes.

This way we don't have to update the footnotes block attributes everytime the order changes.

ellatrix

Nice work.

UX feedback:

It would be nice if the footnote list could link to the block contain the footnote, both in the editor and front-end.
I can unlink the footnotes and edit the link.
I cannot add footnotes to e.g. headings.
The footnote should link to the footnote block.
Would love too see it in the inserter after Add Inline Images and Inline Blocks API #6959.
Does the ID have to be so long? Would be nice to have e.g. #footnote-1 etc.

ellatrix · 2018-06-04T16:52:23Z

core-blocks/footnotes/editor.js

@@ -0,0 +1,76 @@
+/**


Should this file be edit.js like the other blocks?

ellatrix · 2018-06-04T16:54:09Z

core-blocks/footnotes/style.scss

@@ -0,0 +1,9 @@
+.post,
+.edit-post-visual-editor {
+  counter-reset: footnotes;


There are spaces here instead of tabs.

ellatrix · 2018-06-04T16:57:46Z

core-blocks/paragraph/index.js

@@ -140,6 +144,7 @@ class ParagraphBlock extends Component {
 	render() {
 		const {
 			attributes,
+			updateFootnotes,


Ideally the paragraph block is not aware of the footnotes. This should move to RichText instead. You should be able to add footnotes in any RichText area. Also there it would be good to add filters so this could be done by a plugin too (not just footnotes, but adding anything in RichText areas).

ellatrix · 2018-06-04T16:59:52Z

core-blocks/test/fixtures/core__pullquote__multi-paragraph.json

@@ -15,7 +15,7 @@
                                "Paragraph ",
                                {
                                    "type": "strong",
-                                    "key": "_domReact71",
+                                    "key": "_domReact73",


I think you can drop this information from the tests, along with the ref etc. Have a look at some of the others tests, some of which should not include these properties.

ellatrix · 2018-06-04T17:00:50Z

editor/components/block-list/block.js

@@ -60,6 +61,7 @@ export class BlockListBlock extends Component {

 		this.setBlockListRef = this.setBlockListRef.bind( this );
 		this.bindBlockNode = this.bindBlockNode.bind( this );
+		this.updateFootnotes = this.updateFootnotes.bind( this );


Also here, ideally there should be no footnote specific logic in this component.

ellatrix · 2018-06-04T17:02:30Z

editor/components/rich-text/format-toolbar/index.js

@@ -47,6 +47,11 @@ const FORMATTING_CONTROLS = [
 		shortcut: displayShortcut.primary( 'k' ),
 		format: 'link',
 	},
+	{


Looks like we'll need something like #6642 so this can move to the block registration.

ellatrix · 2018-06-04T17:03:51Z

editor/components/rich-text/index.js

+			return;
+		}
+		const uid = uuid();
+		this.editor.insertContent( `<sup data-wp-footnote-id="${ uid }"><a href="#${ uid }" class="wp-footnote"><span class="screen-reader-text">${ __( 'See footnote' ) }</span></a></sup> ` );


Should this be contenteditable=false? Looks like complex markup that should be protected.

Also here we need proper extension points in place. Maybe this could also make use of #6959 when merged.

…r blocks

Aljullu · 2018-06-04T18:02:56Z

Hi @iseulde. Thank you for the input! I already fixed a couple of small things now and will take a deeper look to the other comments in the following days.

Adding contenteditable="false" the user can't edit inside the SUP tag, which makes interacting with footnote numbers much easier because the cursor can't get caught inside its tags.

aduth · 2018-09-13T19:19:59Z

This pull request appears to have languished and will not be easily reconciled with master. Please feel free to reopen and rebase against the current master, or open a new pull request.

xavivars · 2018-11-11T09:17:11Z

Why is this tagged as done by automation? The code has never been merged... I'd reopen it, if possible

Aljullu force-pushed the add/footnotes branch 3 times, most recently from 8c22644 to ee08de9 Compare May 9, 2018 19:51

Aljullu force-pushed the add/footnotes branch from 16559a4 to 9ed6819 Compare May 16, 2018 22:04

nb reviewed May 17, 2018

View reviewed changes

Aljullu force-pushed the add/footnotes branch from 9ed6819 to 817ab53 Compare May 20, 2018 16:07

Aljullu added 18 commits May 21, 2018 01:19

Add footnotes control button to paragraph block

791305a

Adds a button to the toolbar to add footnotes. When clicked, a SUP tag is added the paragraph content. For now, the button is only activated in paragraphs blocks.

Store footnote ids in the block object

533aa51

Everytime the block is modified, it parses its code and stores the footnotes in the block object.

Create Footnotes block

213150e

Create Footnotes block which renders the list of footnotes texts when saved and allows editing them in the post editor.

Use auto-incrementing numbers instead of asterisks for footnotes

2e6de5e

Make footnotes be numbers instead of asterisks and link to the relevant footnote.

Update footnotes block when inserting or removing blocks

40dc686

In the case a block is inserted or removed from the list of blocks, footnotes might change. Because of that, we must update the list of footnotes in those cases.

Make footnotes block to render nothing if there are no footnotes

b5cee89

Until now, the footnotes block was always rendering OL tags even if there were no footnotes.

Rename variables like 'newState' to 'nextState' to keep consistency w…

3941d18

…ith the other functions

Use a template literal for the footnotes markup

55889c2

Use withSelect HOC in Footnotes block so it's automatically updated w…

e8597a0

…hen footnotes change This allows us to clean a lot of code from the reducer.

Clean up parser code no longer being used

ec802ab

Simplify the code which rendersthe footnotes block

d2309af

Create insertFootnotesBlock action to create the footnotes block when…

e7dc2bc

… required This allows us to create the block only if footnotes are present, before we were attaching the footnotes block on editor setup.

Remove footnotes block when the last footnote is deleted

23a3037

Import parseFootnotesFromContent from @wordpress/editor instead of @w…

f519cac

…ordpress/blocks

Update paragraph snapshots with blockFootnotes attribute

e7715cd

Flatten the blocks before searching for footnotes

a9ddeee

Otherwise, getFootnotes wasn't returning the footnotes of inner blocks.

Optimize checking if footnotes block must be added/removed

089dd9d

Only check if footnotes block must be added or removed when the footnotes of a block change. Until now, the check was made everytime a block changed its attributes or was split.

Aljullu force-pushed the add/footnotes branch from bb02bc2 to 089dd9d Compare May 21, 2018 00:00

Aljullu added 3 commits May 21, 2018 03:12

Update footnotes block fixtures

c33beac

Raise paragraph key in pullquote test so it doesn't collide with rece…

8c2a1a2

…ntly added tests A better solution will be needed in order to ignore the key during tests so adding more tests doesn't break the previous ones.

Extract blockFootnotes paragraph attribute from the markup

b3c303d

Aljullu force-pushed the add/footnotes branch from e4fb855 to 948f67c Compare May 21, 2018 16:22

Aljullu added 3 commits May 21, 2018 20:38

Refactor getFootnotes selector

14b9b2d

Code continues working as usual but it has been optimized, made simpler and the dependency to getFlattenedBlocks has been removed, so it no longer has to be stored in the utils.

Several minor fixes and typos corrected

0646693

Don't trigger updateFootnotes when spliting a paragraph block

ac2e5af

Aljullu force-pushed the add/footnotes branch from 948f67c to ac2e5af Compare May 21, 2018 19:12

Aljullu commented May 21, 2018

View reviewed changes

Aljullu force-pushed the add/footnotes branch 2 times, most recently from 55ad109 to ee0f5ef Compare May 23, 2018 00:12

Update footnotes attribute when the order is changed

72277b5

There was a bug that when reordering blocks with footnotes, those were not reordered when saving the post.

Aljullu force-pushed the add/footnotes branch from ee0f5ef to 72277b5 Compare May 23, 2018 00:16

nb reviewed May 23, 2018

View reviewed changes

Order footnotes when saving footnotes block

11794cc

This way we don't have to update the footnotes block attributes everytime the order changes.

ellatrix reviewed Jun 4, 2018

View reviewed changes

Aljullu added 2 commits June 4, 2018 19:56

Rename FootnotesEditor to FootnotesEdit to keep consistency with othe…

c958cca

…r blocks

Replace spaces with tabs in footnotes style sheets

578d71a

Make the footnote number non-editable

400140f

Adding contenteditable="false" the user can't edit inside the SUP tag, which makes interacting with footnote numbers much easier because the cursor can't get caught inside its tags.

Aljullu mentioned this pull request Jun 21, 2018

Remove React-related attributes from pullquote multi-paragraph block fixtures #7448

Closed

3 tasks

designsimply added the [Status] In Progress Tracking issues with work in progress label Jun 27, 2018

aduth closed this Sep 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add footnotes support to Gutenberg #6549

[WIP] Add footnotes support to Gutenberg #6549

Aljullu commented May 3, 2018 •

edited

Loading

nb commented May 9, 2018

ZebulanStanphill commented May 10, 2018

Aljullu commented May 10, 2018

nb commented May 17, 2018

nb commented May 17, 2018 •

edited

Loading

nb commented May 17, 2018

nb May 17, 2018

nb May 17, 2018

Aljullu May 21, 2018

Aljullu May 21, 2018

ellatrix Jun 4, 2018

Aljullu Jun 21, 2018

Aljullu commented May 21, 2018

nb May 23, 2018

Aljullu May 23, 2018

nb May 23, 2018

Aljullu May 23, 2018

Aljullu May 27, 2018

nb May 23, 2018

Aljullu May 23, 2018

Aljullu May 27, 2018

ellatrix left a comment

ellatrix Jun 4, 2018

ellatrix Jun 4, 2018

ellatrix Jun 4, 2018

ellatrix Jun 4, 2018

ellatrix Jun 4, 2018

ellatrix Jun 4, 2018

ellatrix Jun 4, 2018

ellatrix Jun 4, 2018

Aljullu commented Jun 4, 2018

aduth commented Sep 13, 2018

xavivars commented Nov 11, 2018

[WIP] Add footnotes support to Gutenberg #6549

[WIP] Add footnotes support to Gutenberg #6549

Conversation

Aljullu commented May 3, 2018 • edited Loading

Description

Screenshots

Types of changes

User visible changes

Internal changes

How has this been tested?

Checklist

Open bugs

Next steps

nb commented May 9, 2018

ZebulanStanphill commented May 10, 2018

Aljullu commented May 10, 2018

nb commented May 17, 2018

nb commented May 17, 2018 • edited Loading

nb commented May 17, 2018

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

Aljullu commented May 21, 2018

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

ellatrix left a 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

Aljullu commented Jun 4, 2018

aduth commented Sep 13, 2018

xavivars commented Nov 11, 2018

Aljullu commented May 3, 2018 •

edited

Loading

nb commented May 17, 2018 •

edited

Loading