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

[docs] Add "back to top" button #30441

Merged
merged 13 commits into from May 19, 2022
Merged

[docs] Add "back to top" button #30441

merged 13 commits into from May 19, 2022

Conversation

VibhorJaiswal
Copy link
Contributor

@VibhorJaiswal VibhorJaiswal commented Dec 29, 2021

@mui-pr-bot
Copy link

mui-pr-bot commented Dec 29, 2021

No bundle size changes

Generated by 🚫 dangerJS against c5e5345

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

@oliviertassinari oliviertassinari added the on hold There is a blocker, we need to wait label Dec 29, 2021
@VibhorJaiswal VibhorJaiswal marked this pull request as draft December 30, 2021 13:32
@VibhorJaiswal VibhorJaiswal marked this pull request as ready for review December 30, 2021 16:17
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 5, 2022
@danilo-leal danilo-leal changed the title feat: Added "back to top" button. [docs] Add "back to top" button Mar 24, 2022
@danilo-leal danilo-leal added the docs Improvements or additions to the documentation label Mar 24, 2022
@mnajdova mnajdova requested a review from danilo-leal May 18, 2022 08:04
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 18, 2022
@mnajdova
Copy link
Member

mnajdova commented May 18, 2022

@VibhorJaiswal thanks for starting this effort, I've pushed a commit to refactor it a bit. @danilo-leal could you verify the design of the back to top button before we merge?

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

It's a great first pull request on MUI 👌 Thank you for working on it!

@mui-bot
Copy link

mui-bot commented May 18, 2022

No bundle size changes

Generated by 🚫 dangerJS against c8b9389

Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

Sweet 👍

@mnajdova mnajdova merged commit 78bf203 into mui:master May 19, 2022
@oliviertassinari
Copy link
Member

oliviertassinari commented May 19, 2022

We have a bug with the tooltip animation:

strange.tooltip.mp4

However, are we sure about this change? In #30421 I felt that the direction was "thanks for the suggestion, but no thanks".

Maybe it would help to leave a new comment on the issue to explain the rationale to move forward. What I'm most interested in is the rationale around the tradeoff. The main cons I can think about how the features are implemented in this PR, and that I experience is that when I start reading a page, and as soon as I start to scroll down, I get a blue animating on the bottom left of the page that distracts me, it interrupts my reading flow. Maybe a fading instead of a zoom animation would be less distracting?

Would it make sense to do a follow-up to add Google Analytics events to track the usage of the button? That if it has less than 10,000 clicks a month, <0.1% of the page views, then we remove the feature?

Alternatively, the UX I can experience in https://docusaurus.io/docs/advanced/client feel better: there is no strong primary color, and the button is only visible when scrolling up.

Another thought, should the scroll to the top be instant? It's how it behaves in https://docs.github.com/en/account-and-profile/managing-subscriptions-and-notifications-on-github/setting-up-notifications/about-notifications. Does it feel better or worse?


A final thought, the current UX is consistent with what I have built-in https://mui.com/material-ui/react-app-bar/#back-to-top (#17062), having this consistency is great, we use what we promote. But did I do a great job in insight? Maybe it's this demo that we should rethink 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation on hold There is a blocker, we need to wait
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[docs] Add "Back to top" button
6 participants