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.

Sign up for GitHub

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

Jump to bottom

Always use h1 for first heading on a page #3009

Merged
merged 17 commits into from
Jun 18, 2024
Merged

Always use h1 for first heading on a page #3009

merged 17 commits into from
Jun 18, 2024

Conversation

colleenmcginnis
Copy link
Contributor

@colleenmcginnis colleenmcginnis commented Jun 4, 2024 •
edited
Loading

Related to #2768 (specifically #2768 (comment))
Fixes #1614

I think this should force the first heading on every page to be an h1. However, this fix seems to easy so I'm not sure if I'm misunderstanding the total impact of this change. Opening this PR to see if the PR preview builds and what it looks like before fixing tests. Switched to a different approach detailed in #3009 (comment) below.

@colleenmcginnis colleenmcginnis self-assigned this Jun 4, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 4, 2024

A documentation preview will be available soon.

Request a new doc build by commenting
  • Rebuild this PR: run docs-build
  • Rebuild this PR and all Elastic docs: run docs-build rebuild

run docs-build is much faster than run docs-build rebuild. A rebuild should only be needed in rare situations.

If your PR continues to fail for an unknown reason, the doc build pipeline may be broken. Elastic employees can check the pipeline status here.

@colleenmcginnis
Copy link
Contributor Author

colleenmcginnis commented Jun 4, 2024 •
edited
Loading

It looks like this "fix" sometimes results in pages with multiple h1 headings on (I think) pages that don't use [discrete] or [float] on all headings that should be on the same page so I'm not sure it's much of a fix after all... I'm not sure if I should dig in further or rely on just styling the headings appropriately like we're doing in #2768.

@bmorelli25
Copy link
Member

Can you provide a double H1 example? I'm having trouble finding one.

@colleenmcginnis
Copy link
Contributor Author

Here are all of them in the 8.13 version of the Observability guide:

So there are not too many in Observability docs, but in bigger/older books there are many more. For example, in v8.13 of the Elasticsearch guide 843 out of 1815 HTML files contain more than one h1.

@bmorelli25
Copy link
Member

Discussed in metrics mtg this morning. @KOTungseth will ask Katy on Friday if no H1 is better or worse than multiple H1s.

@colleenmcginnis
Copy link
Contributor Author

Ok I tried a different approach here: trying to use the built-in AsciiDoc document header. It looks like we set the noheader option to true. I'm not sure why or if there will be unintended consequences by turning this off, but I haven't come across a dealbreaker... yet.

It's not as simple as changing that option. This PR would also:

  • Move the breadcrumbs. If we use the document header with its h1, the current placement of the breadcrumbs would be below the title. To address this, we can create a placeholder div for the breadcrumbs when we create the header, and then replace the placeholder when we go through the chunk logic. (Note: This is breaking the tests. We need to change where to expect the breadcrumbs.)
  • Hide the first heading in div#content. When we use the document header, the title is still also added in the body of the doc (specifically in div#content), but we can hide that heading with CSS.
  • Still hack incremental heading styles. The logic in this PR would ensure two things: (1) the first heading on each page is an h1 and (2) that will be the only h1 on the page. However, it doesn't guarantee that the headings in the main body of the doc will be incremental, but we can style them appropriately at run time.

@bmorelli25
Copy link
Member

Clicked around a bit.

  • The docs landing page is a bit broken with this new update.
  • It looks like OTP highlighting doesn't work, BUT, I love that the OTP exists on every page now and is even expanded on a lot of pages due to every page starting with H1.

@colleenmcginnis
Copy link
Contributor Author

colleenmcginnis commented Jun 11, 2024 •
edited
Loading

Clicked around a bit.

  • The docs landing page is a bit broken with this new update.
  • It looks like OTP highlighting doesn't work, BUT, I love that the OTP exists on every page now and is even expanded on a lot of pages due to every page starting with H1.

If these are the only problems, that would be good news! For the first, this week we plan to replace the current landing page with a Docsmobile-powered one. And for the second, that is a problem that JavaScript can solve, which is my specialty!

@bmorelli25
Copy link
Member

Awesome!

@colleenmcginnis colleenmcginnis changed the title [TEST] Always use h1 for first heading on a page Always use h1 for first heading on a page Jun 17, 2024
@colleenmcginnis colleenmcginnis marked this pull request as ready for review June 17, 2024 16:43
@colleenmcginnis colleenmcginnis requested a review from a team as a code owner June 17, 2024 16:43
bmorelli25
bmorelli25 approved these changes Jun 17, 2024
View reviewed changes
Copy link
Member

@bmorelli25 bmorelli25 left a comment

Choose a reason for hiding this comment

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

This looks great. Nice fix for that 🐛 in 34a16c6. I've clicked around as much as I can, and I'm not seeing any new 🐛s.

🐦

@colleenmcginnis colleenmcginnis merged commit d16085b into elastic:master Jun 18, 2024
3 checks passed
@colleenmcginnis colleenmcginnis deleted the always-start-with-h1 branch June 18, 2024 14:47
@colleenmcginnis colleenmcginnis mentioned this pull request Jun 18, 2024
Merged
bmorelli25 added a commit to bmorelli25/docs that referenced this pull request Jun 18, 2024
@bmorelli25
Revert "Always use h1 for first heading on a page (elastic#3009)"
940dc9b 
This reverts commit d16085b.
bmorelli25 added a commit that referenced this pull request Jun 18, 2024
@bmorelli25
Revert "Always use h1 for first heading on a page (#3009)" (#3018)
8406230 
This reverts commit d16085b.
bmorelli25 added a commit to colleenmcginnis/docs that referenced this pull request Jun 18, 2024
@bmorelli25
Revert "Revert "Always use h1 for first heading on a page (elastic#3009…
8273fc7 
…)" (elastic#3018)"

This reverts commit 8406230.
bmorelli25 added a commit that referenced this pull request Jun 18, 2024
@colleenmcginnis @bmorelli25
Update name of css and js files (#3017)
0eae7e0 
* update name of css and js files

* fix tests

* Revert "Revert "Always use h1 for first heading on a page (#3009)" (#3018)"

This reverts commit 8406230.

---------

Co-authored-by: bmorelli25 <[email protected]>
colleenmcginnis added a commit to colleenmcginnis/docs that referenced this pull request Jun 20, 2024
@colleenmcginnis
fix formatting issues introduced by elastic#3009
2f3dea9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmorelli25 bmorelli25 bmorelli25 approved these changes

@lcawl lcawl Awaiting requested review from lcawl lcawl is a code owner automatically assigned from elastic/docs-repo-owners

@leemthompo leemthompo Awaiting requested review from leemthompo leemthompo is a code owner automatically assigned from elastic/docs-repo-owners

@kilfoyle kilfoyle Awaiting requested review from kilfoyle kilfoyle is a code owner automatically assigned from elastic/docs-repo-owners

Assignees

@colleenmcginnis colleenmcginnis

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update docs to include a H1 on all pages
2 participants
@colleenmcginnis @bmorelli25