Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

v2 Table of Contents refactor (Revisited) #1832

Merged
merged 27 commits into from
Aug 9, 2023
Merged

Conversation

endigo9740
Copy link
Contributor

@endigo9740 endigo9740 commented Aug 7, 2023

Linked Issue

Closes #1030

Description

I've revisited the approach to implementing the table of contents feature. This now contains three parts:

  1. tocCrawler - action that scans/generates IDs for headings, this populates the tocStore
  2. tocStore - a writable store that accepts a list of heading data to generate the list of links against
  3. TableOfContents - a component that generates the list of links and visible UI for the page

This includes a few other notable changes:

  • We now utilize the native browser inner-page scroll instead of javascript
  • Users can control the smooth scrolling and scroll margin via CSS
  • The action approach is very flexible, allowing you to localize your target region, and easy updates with a key argument

Changsets

Instructions: Changesets automate our changelog. If you modify files in /packages/skeleton, run pnpm changeset in the root of the monorepo, follow the prompts, then commit the markdown file. Changes that add features should be minor while chores and bugfixes should be patch. Please prefix the changeset message with feat:, bugfix: or chore:.

Checklist

Please read and apply all contribution requirements.

  • This PR targets the dev branch (NEVER master)
  • Documentation reflects all relevant changes
  • Branch is prefixed with: docs/, feat/, chore/, bugfix/
  • Ensure Svelte and Typescript linting is current - run pnpm check
  • Ensure Prettier linting is current - run pnpm format
  • All test cases are passing - run pnpm test
  • Includes a changeset (if relevant; see above)

@endigo9740 endigo9740 linked an issue Aug 7, 2023 that may be closed by this pull request
5 tasks
@changeset-bot
Copy link

changeset-bot bot commented Aug 7, 2023

🦋 Changeset detected

Latest commit: aa09ac0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@skeletonlabs/skeleton Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Aug 7, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
skeleton-docs ✅ Ready (Inspect) Visit Preview Aug 9, 2023 9:37pm

@endigo9740 endigo9740 marked this pull request as draft August 7, 2023 18:01
@endigo9740 endigo9740 changed the base branch from dev to v2 August 7, 2023 18:02
@endigo9740
Copy link
Contributor Author

@Mahmoud-zino I've pushed a number of improvements this afternoon, including:

  • Updated the visual styles - very simple by default, use the inactive and active props if you wish to use button styles.
  • Expanded to include additional props, including region props, and indentStyles to specify indent styles (or more)
  • I've opted to embed the fade animation via CSS inside the component. I don't think we need dynamic transitions here.
  • Updated the page documentation to cover the available options and talk you through how this works

I'll aim to revisit and refine a bit more tomorrow.

@endigo9740
Copy link
Contributor Author

@Mahmoud-zino I'm stepping out for the day, but note I just found an odd bug:

Screenshot 2023-08-07 at 4 45 19 PM

  • If I start on any page, refresh the page, the ToC displays as expected
  • If I then browse to a new page, then scroll, a duplicate of the ToC appears

Both the original and duplicate appear to be updated - but I have no idea why it's duplicating these!

I'll pick up here and investigate tomorrow unless you get to this sooner!

@Mahmoud-zino
Copy link
Contributor

Mahmoud-zino commented Aug 8, 2023

also if you add me as a contributor (I don't have write access to this PR) I can help you with some typing stuff. or I can leave comments on them whichever is easier for you.

@endigo9740
Copy link
Contributor Author

@Mahmoud-zino done

@endigo9740
Copy link
Contributor Author

endigo9740 commented Aug 9, 2023

@Mahmoud-zino

when selecting an element from the TOC, the highlight will shift to the next element => it seems the scroll shifts some pixels down causing it to select the next element.

I'm not sure what you mean by this. When I choose any heading or refresh the page with a particular hash in place it aligns directly with the desired heading. It's only when I scroll up or down that it moves - and it's always the expected heading. Can you expand on this?

selecting the last element on some pages (e.g. drawer) will not highlight the last element on the first click. However, clicking the last item again will highlight it.

I noted in "Active on Scroll" section, there's really nothing that can be done. It's up to the page layout to determine how far you can scroll. This is in parity to the previous version, except it now allows you to tap the option and highlight it if you're already near the bottom. The moment a scroll occurs the user selection is gone. I welcome ideas on this, but I think this is as good as it's going to get.

I am not sure if this is a bug or intentionally behaving this way: sometimes when loading a page, there is no highlight, after scrolling the highlight is there.

The auto-highlight only works when a scroll is triggered. If there's no hash, then no ToC links is active by default. This is working as intended.

disable scroll-smooth if the user has PrefersReducedMotion set. (the store is already implemented).

DONE!

Add a changeset

DONE!

@endigo9740
Copy link
Contributor Author

@Mahmoud-zino I've pushed a small fix reinstating the scroll threshold of 40px. This appears to resolve the issue in Firefox where the auto-scroll section is slightly off.

Copy link
Member

@AdrianGonz97 AdrianGonz97 left a comment

Choose a reason for hiding this comment

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

I've pushed a couple of adjustments for types and spelling errors. I've also including a few suggestions/questions below:

…nts/+page.svelte

Co-authored-by: CokaKoala <31664583+AdrianGonz97@users.noreply.github.com>
@endigo9740 endigo9740 marked this pull request as ready for review August 9, 2023 21:28
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Table of Contents refactor
3 participants