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

scroll to top functionality #5620

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

bintus-ux
Copy link

Overview
The Scroll to Top feature enhances the user experience by providing an intuitive way for users to quickly return to the top of the registry page. This is particularly useful for pages with extensive content or long scroll lengths, such as the OpenTelemetry registry page, where users frequently navigate through dense information.

Note: This implementation addresses the same functionality described in an earlier PR (issue number #5331) for the Scroll to Top feature. However, due to issues with submodules in the previous PR, I have created this new PR to ensure proper integration of the feature. The previous PR serves as a reference for the original implementation.

@bintus-ux bintus-ux requested a review from a team as a code owner November 18, 2024 11:52
Copy link

linux-foundation-easycla bot commented Nov 18, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Member

@svrnm svrnm left a comment

Choose a reason for hiding this comment

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

thanks. See some comments

layouts/partials/hooks/body-end.html Outdated Show resolved Hide resolved
layouts/partials/hooks/body-end.html Outdated Show resolved Hide resolved
layouts/shortcodes/ecosystem/registry/search-form.html Outdated Show resolved Hide resolved
@bintus-ux
Copy link
Author

Successfully ran npm run check:format.

@bintus-ux bintus-ux requested a review from svrnm November 19, 2024 12:50
Comment on lines 42 to 59

// Scroll-to-top button styling
.scroll-container {
text-align: center;
margin-top: 20px;
}

.scroll-btn {
background-color: #007bff;
color: white;
bottom: 40px;
right: 40px;
display: none;
box-shadow: 0 4px 8px rgba(0, 0, 0, 0.2);
z-index: 1000;
border-radius: 50%;
position: fixed;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since these styles are no longer confined to the registry, they should be moved. I think that it's fine to move it to the end of assets/scss/_styles_project.scss.

Copy link
Author

Choose a reason for hiding this comment

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

Done. @chalin

@@ -2,3 +2,48 @@
{{ partial "script.html" (dict "src" "js/tracing.js") -}}
{{ end -}}
{{ partial "script.html" (dict "src" "js/navScroll.js") -}}

Copy link
Contributor

Choose a reason for hiding this comment

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

Move all of this extra content into it's own partial, say layouts/partials/scroll-to-top.html.

As for the JS, and I think I mentioned this before, do the following:

  • Move the JS into its own file, say, assets/js/scrollToTop.js
  • Use layouts/partials/script.html to bring in the script content.

Copy link
Author

Choose a reason for hiding this comment

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

But @svrnm said i handle this differently already

Copy link
Member

Choose a reason for hiding this comment

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

@bintus-ux can you give more details on your concerns on @chalin's requests? To me they sound like a further improvement on what we have so far.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah all he just suggested was what i did initially already, i had the scrollToTop.js file in the js folder but you suggested i implement the javascript code on the body-end.html file that way getting rid of the scrollToTop.js file seeing as it was no longer needed, but i am confused now @svrnm

Copy link
Author

Choose a reason for hiding this comment

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

My apologies @svrnm @chalin i took my time to re-read the whole conversation and realized i got it mixed up, totally understand now, sorry for the misunderstanding (i have been feverish the past few days). Implementing the changes right now. Thanks so much.

Copy link
Member

Choose a reason for hiding this comment

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

@bintus-ux no worries, everyone of us is misreading things from time to time and from own experience I know that beign a non-native english speaker adds an additional source of potential misunderstanding, that's why it is always good to ask more questions!

I hope you feel better now!

Copy link
Author

Choose a reason for hiding this comment

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

Thank you so much for the words mentor, and yes i'm recuperating fast!

@opentelemetrybot opentelemetrybot requested a review from a team November 19, 2024 15:50
Copy link
Member

@theletterf theletterf left a comment

Choose a reason for hiding this comment

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

I think it should be hidden in mobile resolutions (using a CSS media declaration to set it it display:none; if width is lower than n pixels)

Also, NN recommends to add a text label, too, and make it appear only after 3 or 4 page scrolls https://www.nngroup.com/articles/back-to-top/

@opentelemetrybot opentelemetrybot requested a review from a team November 19, 2024 15:59
@bintus-ux
Copy link
Author

I think it should be hidden in mobile resolutions (using a CSS media declaration to set it it display:none; if width is lower than n pixels)

Also, NN recommends to add a text label, too, and make it appear only after 3 or 4 page scrolls https://www.nngroup.com/articles/back-to-top/

Well noted, on it

@bintus-ux
Copy link
Author

In this update, I hid the "Top" label text on mobile devices using a CSS media query that sets it to display: none when the screen width is below a specified pixel value (e.g., 768px). Additionally, I implemented functionality where the scroll-to-top button disappears after 2-3 seconds of inactivity, as per your feedback. I also made the text label appear only after the user has scrolled 3-4 pages, following NN's recommendation for improved usability. I would appreciate the team's review please. @svrnm @chalin @theletterf

Registry _ OpenTelemetry - Google Chrome 11_20_2024 1_30_28 PM
Registry _ OpenTelemetry - Google Chrome 11_20_2024 1_31_19 PM

@svrnm
Copy link
Member

svrnm commented Nov 26, 2024

@bintus-ux thanks! A few observations using the preview @ https://deploy-preview-5620--opentelemetry.netlify.app/

  • The "Top" does not show up for me. Also could this read "Go to top" or would this be to crowded?
  • We might want to add an exception for / since it feels off on that page

@bintus-ux
Copy link
Author

@bintus-ux thanks! A few observations using the preview @ https://deploy-preview-5620--opentelemetry.netlify.app/

  • The "Top" does not show up for me. Also could this read "Go to top" or would this be to crowded?
  • We might want to add an exception for / since it feels off on that page

Indeed I initially used the text “Go to top” but it appeared crowded so I improvised

@svrnm
Copy link
Member

svrnm commented Nov 26, 2024

@bintus-ux thanks! A few observations using the preview @ deploy-preview-5620--opentelemetry.netlify.app

  • The "Top" does not show up for me. Also could this read "Go to top" or would this be to crowded?
  • We might want to add an exception for / since it feels off on that page

Indeed I initially used the text “Go to top” but it appeared crowded so I improvised

Top similar to the up arrow might not be conclusive to every user, that's why there is the recommendation to go with "Back to Top"

@bintus-ux
Copy link
Author

@bintus-ux thanks! A few observations using the preview @ https://deploy-preview-5620--opentelemetry.netlify.app/

  • The "Top" does not show up for me. Also could this read "Go to top" or would this be to crowded?
  • We might want to add an exception for / since it feels off on that page

Indeed it doesn’t come up for mobile as I was

@bintus-ux thanks! A few observations using the preview @ deploy-preview-5620--opentelemetry.netlify.app

  • The "Top" does not show up for me. Also could this read "Go to top" or would this be to crowded?
  • We might want to add an exception for / since it feels off on that page

Indeed I initially used the text “Go to top” but it appeared crowded so I improvised

Top similar to the up arrow might not be conclusive to every user, that's why there is the recommendation to go with "Back to Top"

yes indeed I’m on it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants