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

Add table of contents to the integrations pages #13572

Merged

Conversation

axilleas
Copy link
Contributor

@axilleas axilleas commented May 23, 2020

Proposed change

This use the https://github.com/toshimaru/jekyll-toc gem to add
a table of contents (ToC) in the integration pages
for quick content scanning. The ToC is enabled by default for all
integrations pages. If you need to disable it, no_toc: true needs to be
added in the YAML frontmatter.

Here's how it looks. It's pretty barebones, but we can change the CSS if needed.

Screenshot_2020-10-01 Zigbee Home Automation

Type of change

Jekyll related.

Checklist

  • This PR uses the correct branch, based on one of the following:
    • I made a change to the existing documentation and used the current branch.
    • I made a change that is related to an upcoming version of Home Assistant and used the next branch.
  • The documentation follows the Home Assistant documentation standards.

@probot-home-assistant probot-home-assistant bot added in-progress This PR/Issue is currently being worked on needs-rebase The PR has been branched of the wrong base branch or targets an incorrect target branch labels May 23, 2020
@probot-home-assistant probot-home-assistant bot added the next This PR goes into the next branch label May 23, 2020
@axilleas axilleas force-pushed the table-of-contents-implementation branch from fd48c2a to 7b51a0d Compare May 23, 2020 19:53
@probot-home-assistant
Copy link

It seems that this PR is targeted against an incorrect branch. Documentation updates which apply to our current stable release should target the current branch. Please change the target branch of this PR to current and rebase if needed. If this is documentation for a new feature, please add a link to that PR in your description.
(message by DocsTargetBranch)

@axilleas axilleas changed the base branch from next to current May 23, 2020 20:07
@axilleas axilleas force-pushed the table-of-contents-implementation branch from 7b51a0d to 99ac2a3 Compare May 23, 2020 20:08
@klaasnicolaas klaasnicolaas requested a review from frenck May 23, 2020 20:09
@klaasnicolaas klaasnicolaas added current This PR goes into the current branch and removed needs-rebase The PR has been branched of the wrong base branch or targets an incorrect target branch next This PR goes into the next branch labels May 23, 2020
source/_integrations/zha.markdown Outdated Show resolved Hide resolved
Gemfile Outdated Show resolved Hide resolved
Gemfile.lock Outdated Show resolved Hide resolved
@axilleas axilleas force-pushed the table-of-contents-implementation branch from 99ac2a3 to 635494e Compare May 23, 2020 20:12
@frenck
Copy link
Member

frenck commented May 23, 2020

In general, I like the idea. However, with longer titles, it becomes really messy in the sidebar really quickly.

Also I think implementing this for just one specific integration, doesn't make sense?

@axilleas
Copy link
Contributor Author

In general, I like the idea. However, with longer titles, it becomes really messy in the sidebar really quickly.

@frenck Yeah, you're right, but we can maybe fix this with some CSS. Also, I think the benefits outweigh this stylistic issue :)

Also I think implementing this for just one specific integration, doesn't make sense?

Yep, we'll need to add this to all integrations. As I wrote in the PR description:

This PR enables the ToC to the ZHA integration page as an example. If you
accept it, I can work on adding the entries to the rest of the integrations and
also send a follow-up PR describing this new addition in
https://github.com/home-assistant/developers.home-assistant/blob/master/docs/documenting/create-page.md.

@frenck
Copy link
Member

frenck commented May 23, 2020

That is not how we should enable it for all integrations... set it as a default in the Jekyll configuration.

@axilleas axilleas force-pushed the table-of-contents-implementation branch from 19c557f to 635494e Compare May 23, 2020 20:38
@frenck
Copy link
Member

frenck commented May 23, 2020

yeah so let's see what we can do about styling:

image

It can be more compact, e.g., the indenting space between list levels can be reduced a lot to make it work in more cases I guess?

Also the first level can be moved more to the left as well, to align with the text above it.

@axilleas
Copy link
Contributor Author

That is not how we should enable it for all integrations... set it as a default in the Jekyll configuration.

I'll need to figure how to do that. I'll get back to you if I find something.

@frenck
Copy link
Member

frenck commented May 23, 2020

I think we can add it to all pages: https://github.com/home-assistant/home-assistant.io/blob/next/_config.yml#L115

We should however add the TOC to other asides as well in that case.

@axilleas axilleas marked this pull request as draft May 29, 2020 20:27
@axilleas
Copy link
Contributor Author

axilleas commented Jun 1, 2020

I think we can add it to all pages: https://github.com/home-assistant/home-assistant.io/blob/next/_config.yml#L115

I actually tried that but it didn't work. There's a limitation of this plugin, and it only works with posts https://github.com/toshimaru/jekyll-toc#2-advanced-usage.

I'll keep digging.

@frenck
Copy link
Member

frenck commented Jun 1, 2020

Well, it states it works on collections, since most stuff is a collection in our setup, it should work on almost everything.

@axilleas axilleas force-pushed the table-of-contents-implementation branch from 6409472 to 1309644 Compare June 2, 2020 07:55
@axilleas
Copy link
Contributor Author

axilleas commented Jun 2, 2020

Well, it states it works on collections, since most stuff is a collection in our setup, it should work on almost everything.

@frenck you're right, not sure why this is not working 🤔 I submitted an issue with a question upstream toshimaru/jekyll-toc#116.

@axilleas axilleas requested a review from frenck June 15, 2020 19:18
@frenck frenck self-assigned this Jul 14, 2020
@frenck
Copy link
Member

frenck commented Jul 16, 2020

Sorry for the late response, all the styling can be found in the sass folder in the root of the project.

@zsarnett
Copy link
Contributor

I feel like the TOC should only be like 2-3 levels. This would reduce the indent and crazy styling as well. We shouldn't need to get that granular

@axilleas axilleas force-pushed the table-of-contents-implementation branch from e84eea5 to 4f5dd8c Compare July 29, 2020 20:30
@axilleas
Copy link
Contributor Author

👋 I'll get back to the PR soon!

@axilleas axilleas force-pushed the table-of-contents-implementation branch 3 times, most recently from 1ccd15d to d8d31b9 Compare August 28, 2020 21:14
@axilleas
Copy link
Contributor Author

@zsarnett I got the max level to be 3.

@frenck I tried a few CSS files from the sass dir, but I couldn't find the correct one it seems. Not sure where to look.

@Gamester17
Copy link
Contributor

Any progress on this?

@frenck
Copy link
Member

frenck commented Sep 30, 2020

@Gamester17 As you can see, it is not. Why you ask?

@axilleas
Copy link
Contributor Author

Yeah, don't know much about frontend, sorry...

@zsarnett
Copy link
Contributor

@axilleas just add your css to the _paulus.scss file

@axilleas
Copy link
Contributor Author

@axilleas just add your css to the _paulus.scss file

Ah, I'll try that, thanks!

@axilleas axilleas force-pushed the table-of-contents-implementation branch from d8d31b9 to 7142bf4 Compare October 1, 2020 15:57
@axilleas
Copy link
Contributor Author

axilleas commented Oct 1, 2020

@zsarnett thanks to your pointer, I was able to make this work!

@frenck can you give this another look?

@axilleas axilleas force-pushed the table-of-contents-implementation branch from 7142bf4 to c43420e Compare October 28, 2020 09:39
This adds a table of contents (ToC) in the integration pages
for quick content scanning. In order to disable it,
`toc: false` needs to be added in the YAML frontmatter.
@axilleas axilleas force-pushed the table-of-contents-implementation branch from c43420e to 0b40ba1 Compare October 28, 2020 09:48
Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

Adjust styling a tiny bit.

I think a good extension on this, would be making the nav sticky (if you scroll down, it is still available). However, could not get that to work.

I think this is a good first step and ready to rock.

Thanks, @axilleas 👍

@frenck frenck merged commit 125040b into home-assistant:current Nov 16, 2020
@probot-home-assistant probot-home-assistant bot removed the in-progress This PR/Issue is currently being worked on label Nov 16, 2020
@axilleas
Copy link
Contributor Author

I think a good extension on this, would be making the nav sticky (if you scroll down, it is still available). However, could not get that to work.

@frenck agreed! Thanks for merging 🚀

@axilleas axilleas deleted the table-of-contents-implementation branch November 17, 2020 08:47
JJdeVries pushed a commit to JJdeVries/home-assistant.io that referenced this pull request Nov 18, 2020
javicalle pushed a commit to javicalle/home-assistant.io that referenced this pull request Nov 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
current This PR goes into the current branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants