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

docs: Fix table of contents extra headings #6933

Merged
merged 1 commit into from Apr 19, 2022

Conversation

camsteffen
Copy link
Contributor

@camsteffen camsteffen commented Apr 14, 2022

Fix docs table of contents issue where headings inside of a method's description are included. In the example below, it looks like Observable only has two methods.

Before:
image

After:
image

@camsteffen camsteffen changed the title Fix table of contents extra headings docs: Fix table of contents extra headings Apr 14, 2022
@jakovljevic-mladen
Copy link
Member

Hi @camsteffen, thanks for fixing this. This is a very good addition to our docs site. However, I'm seeing in your second screenshot that Examples are not being shown anymore (I also reproduced the issue locally).

Could you please check the Angular docs and try to compare how this issue is solved there so we can fix both issues?

@camsteffen
Copy link
Contributor Author

Hi @jakovljevic-mladen. It was my intention to remove "Example". That heading is part of the docs for "subscribe()" but the position and indentation of "Example" makes it look like it is a top-level heading after "Methods". This confused me when I was reading the docs for Observable. The problem is that the heading levels of the markdown docs for methods don't respect the other heading levels of the page. For example, # Example should be ### Example.

@camsteffen
Copy link
Contributor Author

Hmm I was actually expecting the should have tocList with expect number of TocItems test to fail. I was having trouble running the tests locally so I was gonna lean on github actions.

@jakovljevic-mladen
Copy link
Member

It was my intention to remove "Example".

Aaah, I see. It took me a while to understand the real issue.

Let me break it down for the record. On our official docs web site, what we have is this:
image

Which looks like this:

...
Methods
  lift()
  subscribe()
Example
  Subscribe with an Observer
  Subscribe with functions
  Cancel a subscription
  forEach()
  Example:
  pipe()
  toPromise()

What you wanted to achieve is to exclude this part (which is part of the subscribe() heading):

Example
  Subscribe with an Observer
  Subscribe with functions
  Cancel a subscription

so that we have this:

...
  subscribe()
  forEach()
...

However, this is different to what we currently have in the docs (when I say currently, I mean with the latest commit on the master branch - the latest docs site is unfortunately still not deployed to the official docs site). With the latest changes from master branch, this is what we have:

image

This is still wrong as we've got two headers that have wrong indentation: the Examples header that comes after subscribe() and Example header that comes after forEach().

This is what I would suggest: to change Examples and Example headers from H2 to H4 (by adding two # chars, e.g. ## Example -> #### Example). This would fix the issue without having to change anything in the toc.service.ts file. Could you please try to fix the issue that way (and revert everything from toc.service.ts and toc.service.spec.ts files)?

Hmm I was actually expecting the should have tocList with expect number of TocItems test to fail.

Many of the docs tests are wrong in many ways - they were never too important to us and over time (and many docs Angular app updates), many of them started failing without anyone ever noticing this because running the tests was never included in the pipelines. So, I guess we can ignore docs app tests for now.

I'm not against doing changes in files in the docs_app folder, however, we have to be quite careful with such changes. This change that you did might have fixed one issue, but possibly could have created another one (I'm not saying that it has, but that it could). We don't have much technical nor people power to keep track of the possibly newly created issues as we don't have many contributors as Angular has and we don't have PR preview docs generation so we can open the web site with the changes from PR and roam the site randomly.

I'm always much more in favor of fixing the issue in another place than diverging from the original docs app. Would you agree?

p.s. To test the new changes, you can run the docs generation script, then run the Angular build script and open the app at http://localhost:4200/api/index/class/Observable to verify the issue is fixed.

Markdown headings cannot be too high-level lest they render incorrectly
in the docs app.
@camsteffen
Copy link
Contributor Author

This is what I would suggest: to change Examples and Example headers from H2 to H4 (by adding two # chars, e.g. ## Example -> #### Example). This would fix the issue without having to change anything in the toc.service.ts file. Could you please try to fix the issue that way (and revert everything from toc.service.ts and toc.service.spec.ts files)?

Okay, I have done this. It would be nice to have tests that will ensure this problem doesn't pop up again, but I don't mind doing this for now. I'm a little confused by the state of tests in this repo.

Copy link
Member

@jakovljevic-mladen jakovljevic-mladen left a comment

Choose a reason for hiding this comment

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

LGTM

@jakovljevic-mladen
Copy link
Member

I'm a little confused by the state of tests in this repo.

As I said, this repo does not need to have up to date tests for the docs app - the docs app is written by the Angular team, thus almost every test in the docs app is written by them. Maintaining these tests would require quite some time which we don't have ATM.

Thanks for fixing this, Ben will merge the changes.

@benlesh benlesh merged commit 9e15d75 into ReactiveX:master Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants