goetzrobin / spartan

Cutting-edge tools powering Angular full-stack development.
https://spartan.ng
MIT License
1.44k stars 154 forks source link

refactor(docs): auto-generate toc from current page #92

Closed ajitzero closed 10 months ago

ajitzero commented 10 months ago

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

Which package are you modifying?

N/A

- [ ] accordion - [ ] alert - [ ] alert-dialog - [ ] aspect-ratio - [ ] avatar - [ ] badge - [ ] button - [ ] calendar - [ ] card - [ ] checkbox - [ ] collapsible - [ ] combobox - [ ] command - [ ] context-menu - [ ] data-table - [ ] date-picker - [ ] dialog - [ ] dropdown-menu - [ ] hover-card - [ ] input - [ ] label - [ ] menubar - [ ] navigation-menu - [ ] popover - [ ] progress - [ ] radio-group - [ ] scroll-area - [ ] select - [ ] separator - [ ] sheet - [ ] skeleton - [ ] slider - [ ] switch - [ ] table - [ ] tabs - [ ] textarea - [ ] toast - [ ] toggle - [ ] tooltip - [ ] typography

What is the current behavior?

Closes #91

What is the new behavior?

Does this PR introduce a breaking change?

Other information

For reviewers, the current PR is a suggestion and only makes the change for the Tabs page (see diff), and my suggestion is to remove ng-content entirely (I've retained it if anyone wants to see the DOM diff locally). This approach currently only picks up headings from <spartan-section-sub-heading /> but can be extended if I'm missing some other component.

Edit: In the same Tabs page, we have examples as h3 headings, which I have now updated to be shown in the TOC.

Preview: image

This now almost matches how shadcn/ui looks: image


Here's the DOM diff of ng-content vs @for (I don't know if this is relevant or has any implications):

See Diff

```html

``` ![image](https://github.com/goetzrobin/spartan/assets/19947758/e3ecf5d5-2973-43a7-9d57-65bb0dcf2b26)

Also, if this is acceptable and goes through, we could consider removing <spartan-page-nav /> from all pages entirely and add it in <spartan-page /> conditionally based on some flag. Ideally, for other SSGs like Eleventy based on markdown, this flag would be in the frontmatter section and it would be an opt-in/out (like toc: true in frontmatter) based on the user's preference. If and when we get support for Angular components in markdown and migrate the current pages, we could update this TOC component in-place for styling and replace the implementation to a plugin-based thing.

ajitzero commented 10 months ago

The prettier failure is for tsconfig.base.json and is unrelated to my PR. I can check that in a separate commit/PR if you like, but anyhow, only if this suggestion/PR looks good/reasonable.

Edit: That's weird. The Prettier GitHub Action is passing, but running nx format:write shows a change expected in tsconfig.base.json. We may need a separate fix there to use nx commands instead of manual yarn commands in GitHub Actions.

diff --git a/tsconfig.base.json b/tsconfig.base.json
index df13b84..2538b4a 100644
--- a/tsconfig.base.json
+++ b/tsconfig.base.json
@@ -64,10 +64,10 @@
                        "@spartan-ng/ui-table-helm": ["libs/ui/table/helm/src/index.ts"],
                        "@spartan-ng/ui-tabs-brain": ["libs/ui/tabs/brain/src/index.ts"],
                        "@spartan-ng/ui-tabs-helm": ["libs/ui/tabs/helm/src/index.ts"],
-                       "@spartan-ng/ui-tooltip-brain": ["libs/ui/tooltip/brain/src/index.ts"],
-                       "@spartan-ng/ui-tooltip-helm": ["libs/ui/tooltip/helm/src/index.ts"],
                        "@spartan-ng/ui-toggle-brain": ["libs/ui/toggle/brain/src/index.ts"],
                        "@spartan-ng/ui-toggle-helm": ["libs/ui/toggle/helm/src/index.ts"],
+                       "@spartan-ng/ui-tooltip-brain": ["libs/ui/tooltip/brain/src/index.ts"],
+                       "@spartan-ng/ui-tooltip-helm": ["libs/ui/tooltip/helm/src/index.ts"],
                        "@spartan-ng/ui-typography-helm": ["libs/ui/typography/helm/src/index.ts"],
                        "~/*": ["apps/app/src/*"]
                }
ajitzero commented 10 months ago

Hi @elite-benni - I've updated all pages to use the autogenerated version and removed ng-content for now.

Please review and let me know if you'd like any other changes :)

ajitzero commented 10 months ago

There are the ids missing on the h3 tags

I missed these. I noticed this for the tabs page and the CLI page and only fixed it there. I'll go through the rest and update them.

goetzrobin commented 10 months ago

@ajitzero is this ready to go from your side? If yes, I'll give it a final review and then we can merge it πŸ‘

ajitzero commented 10 months ago

Yes, this is done from my end - please review @goetzrobin ! πŸ˜