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

[HTML theme] add "Navigate to the Top" button #12558

Open
kavania2002 opened this issue Jul 13, 2024 · 31 comments
Open

[HTML theme] add "Navigate to the Top" button #12558

kavania2002 opened this issue Jul 13, 2024 · 31 comments
Assignees
Labels
html theme priority:low type:enhancement enhance or introduce a new feature

Comments

@kavania2002
Copy link

kavania2002 commented Jul 13, 2024

Is your feature request related to a problem? Please describe.
Let's say I'm looking into Debugging Tips in contributing guideline page. It's tiring to roll back to top to get back to heading.

Describe the solution you'd like
A to the top button at the bottom right corner might help and avoid excessive scrolling.

@kavania2002 kavania2002 added the type:proposal a feature suggestion label Jul 13, 2024
@picnixz
Copy link
Member

picnixz commented Jul 13, 2024

While I understand the needs of a Prev/Next, on Firefox, you can already go down with Ctrl+Down keys. I don't think we'll add it for our own theme but you can suggest your idea to Furo (this is the theme used in your example).

at the bottom right corner

In our current theme, this place is already taken by the versioning widget from RTD =/

@chrisjsewell IIRC, you recently added Next/Previous buttons, do you think it makes sense to have such button? (for me, no, because you can have it with basic keyboard features but I'd be happy to hear your thoughts).

@kavania2002
Copy link
Author

I think Furo already has. It kind of have at the top centre while scrolling up.

@chrisjsewell
Copy link
Member

do you think it makes sense to have such button?

100% yes, not everyone knows/wants to learn keyboard shortcuts 😉 plus mobile devices do not have keyboards

And yeh furo already has "jump to top" functionality; haven't had a look at the implementation yet though

@picnixz
Copy link
Member

picnixz commented Jul 13, 2024

I'm personally +0 but if you want to take upon the implementation (or someone else could), I don't mind. I'm currently trying to fix PEP 695 parsing for LaTeX builds

@chrisjsewell
Copy link
Member

Oh yeh it's not a priority, I don't know if I will have time soon for this, but I'm open to it (in the non-intrusive way like implemented in furo)

@picnixz picnixz added type:enhancement enhance or introduce a new feature html theme and removed type:proposal a feature suggestion labels Jul 13, 2024
@picnixz picnixz changed the title [Feature] To the Top Button [HTML theme] add "Navigate to the Top" button Jul 13, 2024
@kavania2002
Copy link
Author

Hey, can I work on this issue? I'm new to this organisation so this might help me to learn some new things.

@picnixz
Copy link
Member

picnixz commented Jul 13, 2024

Yes sure! I'll assign the issue to you!

@kavania2002
Copy link
Author

kavania2002 commented Jul 14, 2024

Where do you suggest adding this (navigate to top) button, besides Previous/Next or like the one furo has (at the top centre)?
And which theme should I add this into, or should I include it in basic?
cc: @picnixz @chrisjsewell

@picnixz
Copy link
Member

picnixz commented Jul 14, 2024

I have very bad suggestions when it comes to design... I would say top right as a floating box, because I don't like something at the top center. And for the themes, I would say all of them should have this feature, except perhaps the most minimal where no features at all is better.

I would however make the feature disabled by default, since this could be disruptive to themes that would have something placed at the position we decide.

@kavania2002
Copy link
Author

kavania2002 commented Jul 14, 2024

Should we attach with Previous | Next | Index?
Maybe this way, other themes won't be affected that much (from design perspective atleast).
What do you say?

And for the themes, I would say all of them should have this feature, except perhaps the most minimal where no features at all is better.

For now, I'll try to add in basic theme and maybe later I'll add in other themes as I'm still figuring out the codebase.

@picnixz
Copy link
Member

picnixz commented Jul 14, 2024

Should we attach with Previous | Next | Top?

If the theme does not have Previous/Next buttons, then I think it's worth it. If there's already a previous/next button, enhance them. In general, the Top (or Bottom) buttons are good if they move at the same as you scroll down/up. By the way, it would be easier to discuss the various proposals with visual examples.

@chrisjsewell @timhoffm You have better insights on designs than me so your feedback would be more valuable than mine I think.

@kavania2002
Copy link
Author

kavania2002 commented Jul 14, 2024

Cool, I'll wait for their feedback and yes it'll be good to manage both previous/next buttons with top/bottom buttons.

@timhoffm
Copy link
Contributor

My 2 cents:

This gives some tips on "Back to top" buttons. Furo and pydata-sphinx-theme follow it mostly.

Additional personal comments:

  • If you have persistent breadcrumbs, you can achieve the "back to top" effect by clicking the last element in the breadcumbs; e.g.
    grafik
  • Placement is notoriously difficult. Often a stable position is only achieved by overlay, which may hide content. There's likely no universal good placement. For example center-bottom would fit well for https://www.sphinx-doc.org, even floating, because we when you scroll down completely, the button would end up between the "prev"/"next" buttons and thus not hide content:
    grafik
    From the hide-content aspect "center-top" is also a good position. While it does hide content there, it's only the content you've already read/scrolled over.

I have no general recommendation on placement it all has pros and cons.

@picnixz
Copy link
Member

picnixz commented Jul 15, 2024

Your 2 cents are worth more than 2 cents. Thank you Tim for the feedback!

@trallard
Copy link

trallard commented Jul 16, 2024

Some thoughts here:

  • this is like mentioned above, mostly helpful for mobile and touch devices
  • placement wise, it seems bottom right is the best, in PST we have it bottom center and it works but can sometimes be obtrusive (not a big accessibility issue though considering the size of the button and the fact that it disappears as soon as the user scrolls down a bit again). Considering that in mobile devices the most commonly used and more accessible areas are the bottom right and bottom center portions of the target I suggest sticking to these vs moving the placement to top left/center/right.
  • this still has to be keyboard focusable for accessibility purposes (so best implementation is to use a proper HTML button to inherit semantic web affordances)
  • agree with this being an opt-in configurable feature

As for where this should take the user:

  1. if there are breadcrumbs then this is the ideal target
  2. if not then the first focusable element in the main landmark would be the next best place

Feel free to ping for follow ups or reviews if needed

@kavania2002
Copy link
Author

Thank you so much @timhoffm. This would be of great help while implementing this feature.

@timhoffm
Copy link
Contributor

I just noted that the button of pydata-sphinx-theme does not work well for iPhone. When scrolling up, safari shows the address bar, which reduces the available screen size and thus hides the button. 🙈

So definitively not easy to get this right.

RPReplay_Final1721153791.mp4

@trallard
Copy link

Interesting @timhoffm - I had never noticed this as I am an Android user and have never had anyone flag this before. Is this standard behavior regardless of the browser being used, though?

Will open an issue in PST tomorrow when I am back online.
But ditto, placement is hard.

@kavania2002
Copy link
Author

kavania2002 commented Jul 16, 2024

Here I'm still figuring out the code and continuously thinking about the correct name, the right place to keep the function (because this code has got some hight standards) and this conversation is now making me more nervous to implement this 😓. Now I'm scared, if I'll be able to do this or not.

@trallard
Copy link

Sorry if this has become a bit overwhelming @kavania2002.

What about getting started with a small prototype in a draft pull request. Don't worry about getting everything right the first time or even to integrate all the details here. Start small with a simple button, and feel free to ping me when you have something, I can help review it and we can iteratively make it better, one small step at a time.
You've got this 💪🏽

@kavania2002
Copy link
Author

Thanks a lot @trallard 🙏 . I'll try my best to create a draft pull request.

@trallard
Copy link

Great looking forward to it ✨

@timhoffm
Copy link
Contributor

timhoffm commented Jul 16, 2024

Interesting @timhoffm - I had never noticed this as I am an Android user and have never had anyone flag this before. Is this standard behavior regardless of the browser being used, though?

@trallard This seems to be specific to Safari on iOS.

Edit: The button is placed at 90vh. It may be that Safari is not changing the viewport but overlaying the address bar. If that's the case we cannot detect the change and it'll be hard to find a suitable solution.

@picnixz
Copy link
Member

picnixz commented Jul 17, 2024

Now I'm scared, if I'll be able to do this or not.

If you want some help on understanding the code base, I can give you some advice. I doubt you will need to touch the Python code itself and will likely only need to touch the CSS and HTML files. Nevertheless, what I can suggest, is that you create some dummy Sphinx project, and modify manually the static css file so that you can see the changes on your web browser more easily rather than going back and forth with rebuilding the docs.

And also, don't worry about the time it can take. We are a small team and we are usually not pressed by deadlines and such, so feel free to familiarize yourself or ask questions. I agree that the code base is scary (it was for me the first time) but hopefully you'll only need to touch not-too-much-scary code

@kavania2002
Copy link
Author

@picnixz Thanks. I think I'm getting the hang of this (not-too-much-scary code part).

@kavania2002
Copy link
Author

Hey @trallard @picnixz, I tried my best with implementing the feature. Could you please look into the same?

@picnixz
Copy link
Member

picnixz commented Jul 19, 2024

I am a bit busy for the next 10 days or so but I'll have a look around mid August (or before if I have time). If I forget about it, feel free ro ping me!

@trallard
Copy link

Yay! Excited to see this PR up.

I have been on and off sick for the last few days.
So will try and get a review done sometime this week or the next one.

@mgeier
Copy link
Contributor

mgeier commented Oct 3, 2024

As I've already mentioned in the PR (#12618 (comment)), I don't think this should be part of the basic theme. I think derived themes should have all the freedom to implement this however they see fit (or not).

Having said that, if there is anything that the basic theme can do to make it more straightforward for derived themes to implement such a feature, I'm all for it!

For example, it would probably be helpful to have a localized string (e.g. "Navigate to top") that can then be re-used by derived themes without needing their own internationalization.

@timhoffm
Copy link
Contributor

timhoffm commented Oct 3, 2024

As I've already mentioned in the PR (#12618 (comment)), I don't think this should be part of the basic theme. I think derived themes should have all the freedom to implement this however they see fit (or not).

As long as derived themes can easily opt out of the default implementation, they still have all the freedom.

@kavania2002
Copy link
Author

So sorry, I was busy with some work. So what things are needed from my side, please let me know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
html theme priority:low type:enhancement enhance or introduce a new feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants