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: add a scroll to top btn #1332

Merged
merged 9 commits into from Jul 2, 2023
Merged

feat: add a scroll to top btn #1332

merged 9 commits into from Jul 2, 2023

Conversation

12rambau
Copy link
Collaborator

Fix #1126

I added a scroll to top btn to the main article page. It's located 2 * header sidebar from the top and centered on the page. Following instrcution from @choldgraf and the bootstrap implementation (https://mdbootstrap.com/snippets/standard/mdbootstrap/2964350#js-tab-view) I manage to get something lightweight and working.

The trigger is simply: "when scolling up and under the header initial bottom". I decided to avoid the hidden pixel and to rely on the header bottom to avoid issue when there is a disclaimer.

let me know what you think. pinging @trallard as you mentioned some a11y issues that I may failed to apply.

@trallard trallard self-requested a review May 22, 2023 09:58
@trallard
Copy link
Collaborator

Ooops just noticed this is waiting for me - I have added it to my backlog @12rambau apologies for the delay

@trallard trallard added the kind: enhancement New feature or request label Jun 20, 2023
@12rambau
Copy link
Collaborator Author

no problems we all have long dashboards, I'm pretty sure i'm late on many other.

Copy link
Collaborator

@trallard trallard left a comment

Choose a reason for hiding this comment

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

Thanks, @12rambau. Since you already use a button element for this, I think it should already have all the correct ARIA added.

I left a couple of comments - nothing blocking.

Also since I could not see the button in the rendered PR is this meant to be enabled by default or would this require user-configuration?

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

This looks great to me - once @trallard is happy I'm happy to merge 👍

12rambau and others added 2 commits June 27, 2023 17:13
Co-authored-by: Tania Allard <taniar.allard@gmail.com>
Co-authored-by: Tania Allard <taniar.allard@gmail.com>
@12rambau
Copy link
Collaborator Author

Also since I could not see the button in the rendered PR is this meant to be enabled by default or would this require user-configuration?

I checked this page and it worked by default as expected. Note that the button only appears if you start scrolling up.

@trallard
Copy link
Collaborator

trallard commented Jun 27, 2023

aha! It took me a while to notice it since it appears right in the middle of the screen and the colour is slightly off the background.

Get_started_with_development_—_PyData_Theme_0_13_4dev0_documentation

So, the current positioning + style is not distinct enough from the surrounding context. (it might need aligning with the new design system - but the issue will, more than likely persist)

I think placing this at the bottom of the page with a more noticeable styling would be a better positioning. See, for example, this website https://ashleemboyer.com/blog/accessible-smooth-scroll-to-top-buttons-with-little-code (I know it is a single-column content page), but the style does make it much easier to spot.

Accessible__Smooth_Scroll-to-top_Buttons_with_Little_Code___Ashlee_M_Boyer

Another example is the gov.uk design system page where the Back to top component is also at the bottom - on their sidebar, but since there is little clutter there, it is easy to see it

Back_link_–_GOV_UK_Design_System

Tagging @smeragoel in case she has any insights to share with us from a UX perspective

@choldgraf
Copy link
Collaborator

I'm +1 on putting it on the bottom. I'm assuming most people will use "up to top" on mobile and if that assumption is right, their thumb is easier to reach the bottom than the top

@trallard
Copy link
Collaborator

trallard commented Jun 28, 2023

Absolutely! I completely forgot about the thumb areas on mobile.

See for ref https://www.smashingmagazine.com/2016/09/the-thumb-zone-designing-for-mobile-users/

@12rambau
Copy link
Collaborator Author

I've moved the button in the last 20% of screen so it's closer to where we expect to find it. I kept some margin in case people get an big footer. I also changed its color to primary. @trallard let me know if it's more visible to you.

@smeragoel
Copy link

I think having it on the bottom is definitely an improvement from having it on top. However, I don't like the fact that the button overlaps existing content, that is always messy from a UX pov. I quite like the gov.uk design system page suggestion made my @trallard.

I think we can modify the button to be on the bottom right of the screen, since the right sidebar is pretty much empty towards the bottom. That way the button will be visible, and we avoid any sort of overlapping.

This article also outlines some guidelines for scroll to top buttons, and they suggest that the bottom right side of the page is the most intuitive.

@12rambau
Copy link
Collaborator Author

thanks @smeragoel for your feedback. As this button needs to be available for small devices how can I add it without overlapping the text ? should I hard code an horizontal position or maintain it in the content area for any devices ?

@drammock
Copy link
Collaborator

drammock commented Jun 30, 2023

I would guess it's related, and it's because those other functions are passing args, not kwargs, and you inserted your new kwarg before n_fft in _mt_spectra. Put your new kwarg at the end.

haha, wrong GitHub tab!

@choldgraf
Copy link
Collaborator

This looks great. One question: for me the button has black text on blue background and it's a bit hard to read. Is it WCAG friendly?

Screenshot_20230701-075321

@trallard
Copy link
Collaborator

trallard commented Jul 1, 2023

Some thoughts

  1. I am still not super convinced of the position - but that could be fixed later on also based on feedback
  2. The button is in the primary colour, right now there are a lot of components and links with the same colour so that button would be failing contrast requirements when overlapping with the links behind it (they are the same colour so there is a 1:1 contrast)

Suggestions:

  • This PR is still using the old colours so it would be best to merge main here to bring in line with the recent changes
  • Change the colour to our secondary colour (that way it should have enough contrast against the background when overlapping over links and such)
  • ensure the text inside the button meets contrast requirements by using the --pst-color-secondary-text variable that is generated based on WCAG contrast of 4.5:1

See https://pydata-sphinx-theme.readthedocs.io/en/latest/user_guide/web-components.html#badges-and-buttons

@choldgraf
Copy link
Collaborator

Ah good point about colors. In that case I think it's better we merge this and get feedback from people rather than getting things perfect now. I'm +1 to merge as is

@trallard
Copy link
Collaborator

trallard commented Jul 1, 2023

I'd say we should at least fix the text colour inside the button - right now it's using the --pst-color-text-base which is inaccessible on both themes (and makes this button different from all the other buttons now on the theme)

Screenshot_20230701-084041.png

@choldgraf
Copy link
Collaborator

As you like, I'll hold off on merging if you prefer to do that fix here instead of a follow up

@trallard
Copy link
Collaborator

trallard commented Jul 1, 2023

Right let's merge and do a quick follow-up PR to fix the colours.

@choldgraf
Copy link
Collaborator

@smeragoel do you wish to hold off on merging or are you ok to merge and iterate?

@12rambau
Copy link
Collaborator Author

12rambau commented Jul 2, 2023

Let me update the branch with the modification suggested and I'll merge it

@smeragoel
Copy link

+1 from me as well to merge it now, and then focus on colours and later positioning!

@12rambau 12rambau merged commit 54311e5 into pydata:main Jul 2, 2023
13 of 14 checks passed
@12rambau 12rambau deleted the scroll branch July 2, 2023 16:58
@12rambau
Copy link
Collaborator Author

12rambau commented Jul 2, 2023

Thanks all for your comments, I've done the color updates and merged. Let's iterate from it afterward

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scroll to top button to quickly jump to the top of the page
5 participants