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

Footnote numbering hooks #3685

Closed
wants to merge 10 commits into from
Closed

Conversation

tg-ephox
Copy link
Contributor

@tg-ephox tg-ephox commented Nov 28, 2017

Description

Exploring using hooks to number/remove numbering from footnotes.

Need to use the following content in a Paragraph block:

<p>
    here is a paragraph! here is a footnote
    <a href="#footnote-123"><sup>[123]</sup></a>
    <strong>and another footnote<a href="#footnote-456"><sup>[456]</sup></a></strong>
</p>

The getSaveContent.extraProps hook works quite well to post-process content however the BlockEdit hook is a little harder to get right.

If I click in the Paragraph block the custom-class-name hook will run first and return an array with the Edit and InspectorControls elements so the footnotes hook needs to be aware of this structure changed by the previous hook...

Is there a better way to manage this?

Also the footnotes hook will need to be able to access redux state, as I'm sure other hooks will, in order to determine the numbering of the footnotes...

How Has This Been Tested?

Screenshots (jpeg or gifs if applicable):

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.

@tg-ephox tg-ephox added the [Status] In Progress Tracking issues with work in progress label Nov 28, 2017
@codecov
Copy link

codecov bot commented Nov 28, 2017

Codecov Report

Merging #3685 into master will decrease coverage by 0.99%.
The diff coverage is 80.95%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #3685    +/-   ##
========================================
- Coverage   38.22%   37.23%    -1%     
========================================
  Files         282      277     -5     
  Lines        6864     6674   -190     
  Branches     1257     1215    -42     
========================================
- Hits         2624     2485   -139     
+ Misses       3566     3532    -34     
+ Partials      674      657    -17
Impacted Files Coverage Δ
blocks/library/paragraph/index.js 23.07% <ø> (ø) ⬆️
element/index.js 100% <ø> (ø) ⬆️
blocks/hooks/index.js 100% <100%> (ø) ⬆️
blocks/hooks/footnotes.js 80% <80%> (ø)
components/panel/color.js 0% <0%> (-100%) ⬇️
editor/components/warning/index.js 0% <0%> (-100%) ⬇️
components/panel/row.js 0% <0%> (-100%) ⬇️
blocks/block-controls/index.js 0% <0%> (-100%) ⬇️
blocks/alignment-toolbar/index.js 16.66% <0%> (-83.34%) ⬇️
blocks/block-alignment-toolbar/index.js 33.33% <0%> (-55.56%) ⬇️
... and 77 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 8777e86...55ce16c. Read the comment docs.

@@ -20,7 +20,14 @@ function BlockEdit( props ) {
Edit = blockType.edit || blockType.save;
}

return applyFilters( 'BlockEdit', <Edit key="edit" { ...editProps } />, props );
const filteredElements = applyFilters( 'BlockEdit', { edit: <Edit key="edit" { ...editProps } />, fragments: [] }, props );
Copy link
Contributor Author

@tg-ephox tg-ephox Nov 29, 2017

Choose a reason for hiding this comment

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

@aduth does it make sense separate the Edit element here so that later hooks can rely on it being an element still (in the shape registered)? I added a fragments key also which works for fills, this could actually be calledfills...

return applyFilters( 'BlockEdit', <Edit key="edit" { ...editProps } />, props );
const filteredElements = applyFilters( 'BlockEdit', { edit: <Edit key="edit" { ...editProps } />, fragments: [] }, props );

if ( filteredElements.edit.type !== Edit ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might be too limiting...

@aduth
Copy link
Member

aduth commented Dec 11, 2017

If I click in the Paragraph block the custom-class-name hook will run first and return an array with the Edit and InspectorControls elements so the footnotes hook needs to be aware of this structure changed by the previous hook...

Is there a better way to manage this?

Yes, @gziolo had explored what I find to be a better approach for handling the component hooks, which is to treat each hook as a higher-order component. In other words, it doesn't return an element (which can take many shapes) but instead returns a new component.

@gziolo has this since been merged? Can you link to the work here?

@aduth
Copy link
Member

aduth commented Dec 11, 2017

The aforementioned work is #3827, where some discussion has already occurred.

return element;
}

export default function footnotes( { addFilter } ) {
Copy link
Member

@gziolo gziolo Dec 12, 2017

Choose a reason for hiding this comment

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

#3827 that @aduth referenced got merged. We no longer pass addFilter from outside. It's now directly imported from @wordpress/hooks. We also updated hook names:

  • getSaveContent.extraProps -> blocks.getSaveContent.extraProps
  • BlockEdit -> blocks.BlockEdit
    Those name may change in the near future. You can also use slashes in namespaces:
  • core-footnotes vs core/footnotes

@tg-ephox
Copy link
Contributor Author

Thanks @aduth the BlockEdit hook returning a HoC works nicely!

@danielbachhuber
Copy link
Member

@tg-ephox @gziolo Is this still relevant / actionable?

@danielbachhuber danielbachhuber added [Status] Needs More Info Follow-up required in order to be actionable. and removed [Status] In Progress Tracking issues with work in progress labels May 22, 2018
@gziolo
Copy link
Member

gziolo commented May 23, 2018

@mtias - is it something we need for 5.0? It looks like an abandoned task.

@gziolo gziolo added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label May 23, 2018
@gziolo
Copy link
Member

gziolo commented Jun 10, 2018

There was no activity in this PR for over 6 months. It looks like it was abandoned. Let’s close it. We can always reopen and rebase it with master branch to align with the current state of Gutenberg.

@gziolo gziolo closed this Jun 10, 2018
@gziolo gziolo deleted the try/footnotes-save-content-hook branch June 10, 2018 05:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Needs More Info Follow-up required in order to be actionable. [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.

4 participants