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

feat(jump-links)!: reimplement in line with PFv4 #2283

Merged
merged 28 commits into from Jan 16, 2023

Conversation

bennypowers
Copy link
Member

@bennypowers bennypowers commented Dec 26, 2022

What I did

  • added @patternfly/pfe-core/controllers/scroll-spy-controller.js
  • reimplemented pfe-jump-links with above, and with styles and APIs in line with PFv4 designs

Testing Instructions

Run the demo locally, and check the Deploy Preview

  • is it accessible?
  • how quickly can you break the scroll spy? under what conditions? give clear repros, please!
  • compare the styles and behaviour to PFv4 and to the Scroll spy demo (Deploy Preview)

Notes to reviewers

Public APIs/HTML

Keep in mind that the primary goal here is to replicate the pfv4 dx, not to provide a strictly html-first element. Nonetheless, the controller tries not to presume anything about the available DOM, so that it could be useful in implementing a light-dom-heavy version of jump-links in the near future (i.e. rh-jump-links or rh-table-of-contents, whathaveyou).

Try to think ahead to what you'd need to implement an HTML-first / noscript-y version of this, and think if the controller as-is would be enough.

Assumptions made

There are, as far as i can tell, two places where the controller makes assumptions about it's host's light DOM, both regarding this concept of "link children". Link children are the elements in the controller host's light-dom which link to the scrollable content headings. when the content scrolls up and down the page, the link child which represents the current heading should become "active".

  1. we assume that link children have an href attribute which contains a hash reference to its heading (i.e. <a href="#foo"> or <pfe-jump-links-item href="#bar">
  2. we assume that it's sufficient to toggle some attribute on the link child when its heading scrolls into view.

If you find these assumptions to be too restrictive, please speak up.

@bennypowers bennypowers linked an issue Dec 26, 2022 that may be closed by this pull request
@changeset-bot
Copy link

changeset-bot bot commented Dec 26, 2022

🦋 Changeset detected

Latest commit: 434527d

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

This PR includes changesets to release 2 packages
Name Type
@patternfly/pfe-jump-links Major
@patternfly/pfe-core 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

@bennypowers bennypowers changed the title feat(jump-links)!: initial commit 1:1 feat(jump-links)!: reimplement in line with PFv4 Dec 26, 2022
@github-actions github-actions bot added work in progress POC / Not ready for review demo Updating demo pages doc functionality Functionality, typically pertaining to the JavaScript. styles An issue or PR pertaining only to CSS/Sass tests Related to testing labels Dec 26, 2022
@github-actions github-actions bot added this to In progress in Workflow Dec 26, 2022
@github-actions github-actions bot added the AT passed Automated testing has passed label Dec 26, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Dec 26, 2022

Deploy Preview for patternfly-elements ready!

Name Link
🔨 Latest commit e838dbf
😎 Deploy Preview https://deploy-preview-2283--patternfly-elements.netlify.app/

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions github-actions bot added the tools Development and build tools label Dec 26, 2022
@bennypowers bennypowers mentioned this pull request Dec 26, 2022
@bennypowers bennypowers self-assigned this Dec 28, 2022
@bennypowers bennypowers marked this pull request as ready for review December 28, 2022 12:47
@bennypowers bennypowers enabled auto-merge (squash) December 29, 2022 14:49
@bennypowers
Copy link
Member Author

Thanks @nikkimk ! This should probably get a changeset for adding roving tabindex controller

@brianferry
Copy link
Collaborator

For the scrollspy demo locally I am not able to navigate with my keyboard to any of the headings other than 1 & 5 (first and last) using the arrow keys. Not sure if that is expected or not but didn't see a disabled attribute on those elements.

@zeroedin
Copy link
Collaborator

zeroedin commented Jan 13, 2023

For the scrollspy demo locally I am not able to navigate with my keyboard to any of the headings other than 1 & 5 (first and last) using the arrow keys. Not sure if that is expected or not but didn't see a disabled attribute on those elements.

Not sure what you are seeing @brianferry but FWIW I am able to navigate fine with the arrows, home, and end key all working as expected.

bennypowers and others added 2 commits January 14, 2023 21:21
Co-authored-by: Steven Spriggs <steven.spriggs@gmail.com>
Copy link
Collaborator

@zeroedin zeroedin left a comment

Choose a reason for hiding this comment

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

LGTM

Workflow automation moved this from Ready: code & design review to Approved Jan 16, 2023
@bennypowers bennypowers merged commit 82da44c into main Jan 16, 2023
9 checks passed
Workflow automation moved this from Approved to Done Jan 16, 2023
@bennypowers bennypowers deleted the 2050-11-pfe-jump-links branch January 16, 2023 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AT passed Automated testing has passed demo Updating demo pages functionality Functionality, typically pertaining to the JavaScript. styles An issue or PR pertaining only to CSS/Sass tests Related to testing tools Development and build tools work in progress POC / Not ready for review
Projects
Development

Successfully merging this pull request may close these issues.

[1:1] pfe-jump-links
5 participants