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 scoll to top feature to website #2080

Merged
merged 13 commits into from
May 20, 2024
Merged

Add scoll to top feature to website #2080

merged 13 commits into from
May 20, 2024

Conversation

Dhaulagiri
Copy link
Contributor

@Dhaulagiri Dhaulagiri commented May 7, 2024

πŸ“Œ Summary

This PR introduces a feature to the website that affords users a button that can be clicked to return to the top of the current page.

πŸ› οΈ Detailed description

Implementation choices:

  • Only appears after user has scrolled some (configurable) distance down the page
  • Appears on any page using the <Doc::Page::Content> component, which is pretty much every page except the homepage

πŸ“Έ Screenshots

scroll

πŸ”— External links

Jira ticket: HDS-3298
Figma file


πŸ’¬ Please consider using conventional comments when reviewing this PR.

Copy link

vercel bot commented May 7, 2024

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

Name Status Preview Updated (UTC)
hds-showcase βœ… Ready (Inspect) Visit Preview May 20, 2024 8:00pm
hds-website βœ… Ready (Inspect) Visit Preview May 20, 2024 8:00pm

@Dhaulagiri Dhaulagiri changed the title add ScrollToTop Add scoll to top feature to website May 9, 2024
@Dhaulagiri Dhaulagiri marked this pull request as ready for review May 9, 2024 17:15
@alex-ju
Copy link
Member

alex-ju commented May 15, 2024

@heatherlarsen the CSS linter seems unhappy after the last commit, but you can fix it by running yarn lint:css:fix in website

Copy link
Member

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

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

Looking good!

Copy link
Contributor

@didoo didoo left a comment

Choose a reason for hiding this comment

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

Added a few comments and suggestions.

website/app/components/doc/scroll-to-top/index.hbs Outdated Show resolved Hide resolved
website/app/components/doc/scroll-to-top/index.hbs Outdated Show resolved Hide resolved
website/app/styles/doc-components/scroll-to-top.scss Outdated Show resolved Hide resolved
website/app/styles/doc-components/scroll-to-top.scss Outdated Show resolved Hide resolved
website/app/styles/doc-components/scroll-to-top.scss Outdated Show resolved Hide resolved
website/app/components/doc/scroll-to-top/index.js Outdated Show resolved Hide resolved
website/app/components/doc/scroll-to-top/index.js Outdated Show resolved Hide resolved
website/app/components/doc/scroll-to-top/index.js Outdated Show resolved Hide resolved
website/app/components/doc/scroll-to-top/index.js Outdated Show resolved Hide resolved
website/app/components/doc/scroll-to-top/index.js Outdated Show resolved Hide resolved
heatherlarsen and others added 6 commits May 17, 2024 12:57
- Adjusted the aria label to be more descriptive
- Adjusted the background color to use transparency so it's more obvious that it's overlaying content in smaller devices
- Added box-shadows to the button
- Added a visible change on hover
- Added a subtle transition
Co-authored-by: Cristiano Rastelli <cristiano.rastelli@hashicorp.com>
Copy link
Member

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

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

The approach to always have the button in the DOM with opacity 0 has usability flaws when the button overlaps another interactive element, such as a link on the page (see the example below where it partly overlaps with a doc card image). When the user clicks the area highlighted in red nothing will happen – as you can imagine the problem becomes more acute on narrow screens.

Screenshot 2024-05-20 at 10 15 04

@didoo
Copy link
Contributor

didoo commented May 20, 2024

The approach to always have the button in the DOM with opacity 0 has usability flaws when the button overlaps another interactive element, such as a link on the page (see the example below where it partly overlaps with a doc card image). When the user clicks the area highlighted in red nothing will happen – as you can imagine the problem becomes more acute on narrow screens.

You're right, I didn't think of this case. Do you reckon that adding a pointer-events: none; by default, and removing this style on doc-scroll-to-top--is-visible would be enough?

@alex-ju
Copy link
Member

alex-ju commented May 20, 2024

The approach to always have the button in the DOM with opacity 0 has usability flaws when the button overlaps another interactive element, such as a link on the page (see the example below where it partly overlaps with a doc card image). When the user clicks the area highlighted in red nothing will happen – as you can imagine the problem becomes more acute on narrow screens.

You're right, I didn't think of this case. Do you reckon that adding a pointer-events: none; by default, and removing this style on doc-scroll-to-top--is-visible would be enough?

I've not tested pointer-events, it may allow the click to go through the button and reach the underneath target, but the issue I see with the button being in the DOM when not visible (in contrast with the original implementation) is that when navigating using keyboard and reaching that element the focus is lost (as in, it's on the button, but not visible, hence visually lost), violating WCAG criterion 2.4.7.

…eclarations)

probably not a big difference, but just in case
@didoo
Copy link
Contributor

didoo commented May 20, 2024

I've not tested pointer-events, it may allow the click to go through the button and reach the underneath target, but the issue I see with the button being in the DOM when not visible (in contrast with the original implementation) is that when navigating using keyboard and reaching that element the focus is lost (as in, it's on the button, but not visible, hence visually lost), violating WCAG criterion 2.4.7.

@alex-ju see my last commit: does this address all the issues?

@alex-ju alex-ju dismissed their stale review May 20, 2024 17:01

Dismissed the request for changes as they were addressed

@alex-ju
Copy link
Member

alex-ju commented May 20, 2024

I've not tested pointer-events, it may allow the click to go through the button and reach the underneath target, but the issue I see with the button being in the DOM when not visible (in contrast with the original implementation) is that when navigating using keyboard and reaching that element the focus is lost (as in, it's on the button, but not visible, hence visually lost), violating WCAG criterion 2.4.7.

@alex-ju see my last commit: does this address all the issues?

yes, this looks conformant! πŸ‘Œ

Copy link
Contributor

@heatherlarsen heatherlarsen left a comment

Choose a reason for hiding this comment

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

Thanks everyone for the support and reviews ❀️

Copy link
Contributor

@didoo didoo left a comment

Choose a reason for hiding this comment

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

LGTM πŸ‘

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

5 participants