Add missing headers in the Table of Contents #1916

sirreal · 2017-07-15T21:45:43Z

Add missing headers to the Table of Contents.
Separate TableOfContentsItem into independent component.

Closes #1903

mtias · 2017-07-24T12:44:51Z

@afercia @jasmussen do you think showing missing headings would be better than just highlighting the parent-less level? One concern is that it sort of implies that you should add the missing heading levels instead of changing the incorrect one.

mtias · 2017-07-24T12:45:43Z

editor/sidebar/table-of-contents/item.js

+	<div
+		className={ classnames(
+			'table-of-contents__item',
+			`is-H${ level }`,


This is from the original PR, but we should lowercase the class name.

👍 afe3467

jasmussen · 2017-07-24T12:57:36Z

do you think showing missing headings would be better than just highlighting the parent-less level? One concern is that it sort of implies that you should add the missing heading levels instead of changing the incorrect one.

I agree, the obvious course of action is to simply change the sub heading to be one less deep. Could we show a little exclamation mark or warning triangle or something, which on hover can suggest, perhaps even with a button, to fix the level?

mtias · 2017-07-24T13:02:33Z

Let's start with highlighting with color and adding some text after the headline with "Should be Hx level". @sirreal would you be able to update this one?

afercia · 2017-07-24T13:29:12Z

Or just change the text from "Missing header level" to "Skipped heading level"?
Note: header is a different thing from heading.

mtias · 2017-07-24T14:01:37Z

What I mean is that presentation wise it's the difference between:

h1 Some text
  h2 ...
    h3 ...
      h4 ...
        h6 My other heading

And:

h1 Some text
  h6 My other heading
       (Incorrect heading level)

sirreal · 2017-07-24T14:22:05Z

Or just change the text from "Missing header level" to "Skipped heading level"?
Note: header is a different thing from heading.

👍 @afercia Thanks! Replaced "header" with "heading" in 21b98c3

sirreal · 2017-07-24T14:44:15Z

@mtias I've reworked the component a bit to provide the incorrect level message. Here's what it looks like now:

afercia · 2017-08-02T18:15:02Z

The messages look correct and informative to me. The HTML in the meta box is all <div>s, it would be nice to use some slightly more semantic HTML. Even just <p>s and <span>s would be a bit better 🙂

It would be great if the outline report could update live while editing the post headings! Right now, after you make a change you need to save the post and refresh to see the outline updated. Not sure about technical details and maybe this could be merged as a first iteration" @sirreal thanks! and @mtias what's left to be done here?

Re: the build failing, I think it's the PHP 5.2 thing and just needs to be rebased?

mtias · 2017-08-02T18:24:41Z

Right now, after you make a change you need to save the post and refresh to see the outline updated.

It should update as soon as you focus-out from the heading you were writing. Does that work for you or you still need to save?

mtias · 2017-08-02T18:25:18Z

Also, I think we can remove the "experimental" wording :)

afercia · 2017-08-02T19:44:05Z

It should update as soon as you focus-out from the heading you were writing.

No, it doesn't for me. It does update when deleting a heading or adding a new one.

mtias · 2017-08-02T21:18:25Z

I'll check that out.

This is a functional first attempt at adding missing headers to the Table of Contents. The implementation appears to be correct but could use some clean up.

codecov · 2017-08-03T11:00:29Z

Codecov Report

Merging #1916 into master will increase coverage by 0.77%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1916      +/-   ##
==========================================
+ Coverage   22.15%   22.93%   +0.77%     
==========================================
  Files         138      139       +1     
  Lines        4283     5136     +853     
  Branches      723      967     +244     
==========================================
+ Hits          949     1178     +229     
- Misses       2814     3250     +436     
- Partials      520      708     +188

Impacted Files	Coverage Δ
editor/sidebar/table-of-contents/index.js	`0% <0%> (ø)`	⬆️
editor/sidebar/table-of-contents/item.js	`0% <0%> (ø)`
editor/selectors.js	`95.81% <0%> (-0.49%)`	⬇️
editor/modes/visual-editor/block.js	`0% <0%> (ø)`	⬆️
editor/header/tools/preview-button.js	`0% <0%> (ø)`	⬆️
editor/sidebar/last-revision/index.js	`0% <0%> (ø)`	⬆️
editor/modes/visual-editor/block-list.js	`0% <0%> (ø)`	⬆️
editor/post-title/index.js	`0% <0%> (ø)`	⬆️
components/dropdown-menu/index.js	`93.16% <0%> (+0.1%)`	⬆️
... and 9 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 2beeb4c...b0159e8. Read the comment docs.

sirreal · 2017-08-03T12:26:49Z

@mtias @afercia The issue with the Table of Contents not being updated and the test failures appear to be corrected after rebase.

afercia · 2017-08-03T13:24:14Z

@sirreal thanks! I was just testing it and seems to update now. There's a small CSS issue in Safari and Edge (see screenshot below), and I'd like to try a couple changes to improve a11y (I'm trying them in a local branch):

make the TOC list a <ul>: screen readers can announce how many items are in the list
wrap the CSS generated dashes within a span with aria-hidden="true" because some screen readers read out CSS generated content, for example before the H2 VoiceOver reads out "em dash". See also https://core.trac.wordpress.org/ticket/40428

From left to right: Safari, Edge, and (for reference) IE 11

N.B. the multiple scrollbars is a separate issue

afercia · 2017-08-03T13:58:33Z

If I'm not wrong headings.length > 1 won't work in the case there's just one heading. Was this intentional? /cc @mtias

mtias · 2017-08-03T14:01:49Z

@afercia yes, I thought it didn't make a lot of sense to show it only for one header. My plan was to not show the section at all if you don't have any headers as well.

afercia · 2017-08-03T14:03:26Z

@mtias OK that also avoids the need to handle singular/plural forms 1 Heading / n Headings.

mtias · 2017-08-03T14:08:02Z

editor/sidebar/table-of-contents/index.js

@@ -1,9 +1,8 @@
 /**
- * External dependencies
+ * External Dependencies


"dependencies" should be lowercase

afercia · 2017-08-03T15:12:50Z

I went ahead:

shows the TOC section only when there are at least 2 headings
makes the list of headings a list
hides the CSS generated "dashes" from assistive technologies
fixes flexbox layout for Safari and Edge

Worth noting Safari + VoiceOver don't announce styled lists as lists, this is a Safari bug and the only way to fix it would be to add a redundant role="list" on the <ul> element (and disable a jsx-a11y rule).

Firefox + NVDA announcing the number of items in the list:

Chrome + VoiceOver announcing the number of items in the list:

mtias · 2017-08-03T15:29:16Z

@sirreal feel free to merge when you think it's ready.

mtias · 2017-08-03T15:59:21Z

Thanks both!

sirreal added the [Status] In Progress Tracking issues with work in progress label Jul 15, 2017

sirreal self-assigned this Jul 15, 2017

sirreal added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Enhancement A suggestion for improvement. labels Jul 15, 2017

sirreal changed the title ~~[WIP] Add missing headers in the Table of Contents~~ Add missing headers in the Table of Contents Jul 20, 2017

sirreal removed the [Status] In Progress Tracking issues with work in progress label Jul 20, 2017

mtias reviewed Jul 24, 2017

View reviewed changes

sirreal added 8 commits August 3, 2017 12:53

Implement missing headers in the Table of Contents

f173933

This is a functional first attempt at adding missing headers to the Table of Contents. The implementation appears to be correct but could use some clean up.

Separate TableOfContentsItem into separate component

41cadb3

Refactor TableOfContents for clarity

bd38e57

Use lowercase h* for className

17f7228

Replace header with heading

28a6711

Display "incorrect" message not all missing levels

aeccafe

Rework structure and style to accommodate level msg

095803c

Use map, forEach is no longer necessary

65f9cd1

sirreal force-pushed the add/toc-skipped-headers branch from a0f5938 to 65f9cd1 Compare August 3, 2017 10:55

Remove "experimental" from Table of Contents title

992623c

mtias reviewed Aug 3, 2017

View reviewed changes

Improve a11y and show the TOC when there are at least 2 headings.

b0159e8

sirreal merged commit 360a5ad into master Aug 3, 2017

sirreal deleted the add/toc-skipped-headers branch August 3, 2017 15:49

aduth mentioned this pull request Sep 20, 2017

Add word and block count to table of contents #2684

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing headers in the Table of Contents #1916

Add missing headers in the Table of Contents #1916

sirreal commented Jul 15, 2017 •

edited

Loading

mtias commented Jul 24, 2017

mtias Jul 24, 2017

sirreal Jul 24, 2017

jasmussen commented Jul 24, 2017

mtias commented Jul 24, 2017

afercia commented Jul 24, 2017

mtias commented Jul 24, 2017

sirreal commented Jul 24, 2017

sirreal commented Jul 24, 2017

afercia commented Aug 2, 2017

mtias commented Aug 2, 2017

mtias commented Aug 2, 2017

afercia commented Aug 2, 2017

mtias commented Aug 2, 2017

codecov bot commented Aug 3, 2017 •

edited

Loading

sirreal commented Aug 3, 2017

afercia commented Aug 3, 2017

afercia commented Aug 3, 2017

mtias commented Aug 3, 2017

afercia commented Aug 3, 2017

mtias Aug 3, 2017

afercia commented Aug 3, 2017

mtias commented Aug 3, 2017

mtias commented Aug 3, 2017

Add missing headers in the Table of Contents #1916

Add missing headers in the Table of Contents #1916

Conversation

sirreal commented Jul 15, 2017 • edited Loading

mtias commented Jul 24, 2017

mtias Jul 24, 2017

Choose a reason for hiding this comment

sirreal Jul 24, 2017

Choose a reason for hiding this comment

jasmussen commented Jul 24, 2017

mtias commented Jul 24, 2017

afercia commented Jul 24, 2017

mtias commented Jul 24, 2017

sirreal commented Jul 24, 2017

sirreal commented Jul 24, 2017

afercia commented Aug 2, 2017

mtias commented Aug 2, 2017

mtias commented Aug 2, 2017

afercia commented Aug 2, 2017

mtias commented Aug 2, 2017

codecov bot commented Aug 3, 2017 • edited Loading

Codecov Report

sirreal commented Aug 3, 2017

afercia commented Aug 3, 2017

afercia commented Aug 3, 2017

mtias commented Aug 3, 2017

afercia commented Aug 3, 2017

mtias Aug 3, 2017

Choose a reason for hiding this comment

afercia commented Aug 3, 2017

mtias commented Aug 3, 2017

mtias commented Aug 3, 2017

sirreal commented Jul 15, 2017 •

edited

Loading

codecov bot commented Aug 3, 2017 •

edited

Loading