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

Implemented nesting support in document outline. #5314

Merged
merged 1 commit into from Apr 6, 2018

Conversation

@jorgefilipecosta
Member

jorgefilipecosta commented Feb 28, 2018

Before nested headings were totally ignored now we show them with a representation of their nesting path.
This PR also updates TableOfContentsPanel counting methodology to count nested blocks.
Two small functions computeOutlineHeadings, and blockCount were implemented, should we expose them as an util? I decided not to do so because I don't see other usages for them and if the usages appear later we can always extract them.

How Has This Been Tested?

Verify documented outline and the countings panel work as expected, e.g: the warnings are still displayed and the countings are correct.
Try to nest headings inside columns and verify they appear in the document outline, with their nesting path displayed.

Screenshots (jpeg or gifs if applicable):

image

@mcsf

This implementation looks like a good start. I'm wondering, though, if there is some parent issue or any discussion somewhere on this topic.

A couple of thoughts on my mind:

  • Will there be justifiable scenarios where heading levels have different weights by virtue of belonging to a nested context?
  • What does nesting (and notably multidimensional nesting) mean for the flow of the document, and thus its outline?
isEmpty: isEmptyHeading( block ),
};
}
return block.innerBlocks && block.innerBlocks.length ?

This comment has been minimized.

@mcsf

mcsf Mar 1, 2018

Contributor

Better with lodash#isEmpty

This comment has been minimized.

@jorgefilipecosta
expect( wrapper.contains( 'Heading child' ) ).toBeTruthy();
} );
} );

This comment has been minimized.

@mcsf

mcsf Mar 1, 2018

Contributor

Maybe a slightly more complex test would be a good complement.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Mar 5, 2018

Member

The test case was extended 👍

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Mar 5, 2018

Hi @mcsf,

A couple of thoughts on my mind:

Will there be justifiable scenarios where heading levels have different weights by virtue of belonging to a nested context?
What does nesting (and notably multidimensional nesting) mean for the flow of the document, and thus its outline?

These are really interesting questions. W3.org has a section about outlines and they even provide an algorithm to correctly implement them https://www.w3.org/TR/2011/WD-html5-20110525/sections.html#outlines.

As a very short summary, headings inside these elements "blockquote body details fieldset figure td" (section root) should not count or appear in the parent document outline.
Nesting inside these elements "article aside nav section" (section content) scopes the headings e.g: they should start again from level 1 but they are lower one level the level of the parent of this element.

Provided right now we can not nest headings inside section root or section content, from the outline algorithm point of view there is no nesting. So it looks like right now nesting does not change weights. We are just showing the nesting path in the outline as a way to make the structure more intuitive to the user, and make it easier to navigate in the document.

As nesting involves we should keep eye on this.

@jorgefilipecosta jorgefilipecosta requested a review from WordPress/gutenberg-core Mar 8, 2018

isEmpty: isEmptyHeading( block ),
};
}
return isEmpty( block.innerBlocks ) ?

This comment has been minimized.

@aduth

aduth Mar 14, 2018

Member

Is the isEmpty check necessary? flatMap on an empty array should return an empty array anyways.

@@ -28,6 +30,11 @@ const TableOfContentsItem = ( {
onClick={ onClick }
>
<span className="document-outline__emdash" aria-hidden="true"></span>
{ path.map( ( { name }, index ) => (

This comment has been minimized.

@aduth

aduth Mar 14, 2018

Member

Tab before parenthesis should be a space.

@@ -58,8 +56,20 @@ function TableOfContentsPanel( { blocks } ) {
);
}
const blockCount = ( blocksArray = [], name ) => reduce(

This comment has been minimized.

@aduth

aduth Mar 14, 2018

Member

Should this be a propers elector?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Mar 14, 2018

Member

Yes, it is a good idea and has some advantages. Namely, we can make use of the state.editor.present.blocksByUid structure which is flat so we can have a much simpler and efficient algorithm when compared with the previous recursive version.

( count, block ) =>
count +
( ! name || block.name === name ? 1 : 0 ) +
( block.innerBlocks ? blockCount( block.innerBlocks, name ) : 0 ),

This comment has been minimized.

@aduth

aduth Mar 14, 2018

Member

This logic is excessively complex to be inlined without comments or some named variable / separate composable function.

@jorgefilipecosta jorgefilipecosta requested review from WordPress/gutenberg-core and removed request for WordPress/gutenberg-core Mar 14, 2018

@jorgefilipecosta jorgefilipecosta requested a review from WordPress/gutenberg-core Mar 22, 2018

const isEmptyHeading = heading => ! heading.attributes.content || heading.attributes.content.length === 0;
export const DocumentOutline = ( { blocks = [], title, onSelect } ) => {
const headings = filter( blocks, ( block ) => block.name === 'core/heading' );
const headings = computeOutlineHeadings( blocks );

This comment has been minimized.

@gziolo

gziolo Mar 26, 2018

Member

As discussed on Slack today. Can we convert this functionality (computeOutlineHeadings ) into a general purpose selector getBlocksOfType?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Mar 27, 2018

Member

Unfortunately, at the time of our discussion, I did not remember that here we don't want just the heading blocks we also want the path to it in order to display it to the user.
The path is the array of ancestors of a block until a heading. So it can be ["Core/collumn"] or [] or ["cover/image", "Core/collumn"].
I think this is the only place where we need to retrieve the blocks of a given type sorted by document position with its ancestors so this function is probably not reusable elsewhere.

@@ -28,6 +30,11 @@ const TableOfContentsItem = ( {
onClick={ onClick }
>
<span className="document-outline__emdash" aria-hidden="true"></span>
{ path.map( ( { name }, index ) => (

This comment has been minimized.

@gziolo

gziolo Mar 26, 2018

Member

What does path mean here?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Mar 27, 2018

Member

Hi @gziolo, I just described what path is in comment #5314 (comment).

This comment has been minimized.

@gziolo

gziolo Mar 27, 2018

Member

Yes, but we need to have it better explained in the code for the future us editing this code :)

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Mar 27, 2018

Member

Yes, you are right I added comments on this line and for the function computeOutlineHeadings where the path is computed :)

@@ -439,6 +440,22 @@ export const getBlocks = createSelector(
]
);
export const getGlobalBlockCount = createSelector(

This comment has been minimized.

@gziolo

gziolo Mar 26, 2018

Member

JSDoc is missing here.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Mar 27, 2018

Member

Thank you for catching this it was fixed.

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Apr 2, 2018

This PR was rebased and should be ready to merge soon If we don't find any blocking point.

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Apr 4, 2018

Hi @karmatosed could you please do a final verification regarding the UX/design before the merge :)?

@karmatosed

This comment has been minimized.

Member

karmatosed commented Apr 6, 2018

I think for now this is good but also agree with this:

As nesting involves we should keep eye on this.

We should really see and be ok revisiting this UI also.

@aduth

aduth approved these changes Apr 6, 2018

* isEmpty - Flag indicating if the heading has no content.
*
* @param {?Array} blocks An array of blocks.
* @param {?path} path An array of blocks that are ancestors of the blocks passed as blocks.

This comment has been minimized.

@aduth

aduth Apr 6, 2018

Member

JSDoc type should be ?Array

// This mapping renders each ancestor to make it easier for the user to know where the headings are nested.
path.map( ( { name }, index ) => (
<strong key={ index } className="document-outline__level">
{ getBlockType( name ).title }

This comment has been minimized.

@aduth

aduth Apr 6, 2018

Member

Minor: There is a BlockTitle component now, which could either be accessed by its connected form (pass UID), or its raw form taking name as a prop.

https://github.com/WordPress/gutenberg/tree/master/editor/components/block-title

Implemented nesting support in document outline.
Before nested headings were totally ignored now we show them with a representation of their nesting path.

@jorgefilipecosta jorgefilipecosta merged commit 80185d7 into master Apr 6, 2018

2 checks passed

codecov/project 44.58% (+0.08%) compared to ab8ddc2
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the add/nesting-support-document-outline branch Apr 6, 2018

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Apr 6, 2018

Thank you for this last verifications @karmatosed and @aduth!

@paulwilde

This comment has been minimized.

Contributor

paulwilde commented Apr 6, 2018

Yeah seems the UI would need to be redesigned if the nesting goes beyond 2 levels deep. Here is an example of an actual website (with the text changed to Lorem ipsum) which uses columns nested in sections:

screen shot 2018-04-06 at 18 49 11

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