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

refactor: Adjust styling of back top top button #5469

Merged
merged 10 commits into from
Sep 11, 2021
Merged

refactor: Adjust styling of back top top button #5469

merged 10 commits into from
Sep 11, 2021

Conversation

dsmmcken
Copy link
Contributor

@dsmmcken dsmmcken commented Sep 2, 2021

Motivation

  1. The new back to top button is in the same location as is common for live chat/help widgets, and there is no easy way to fix that without swizzling the "unsafe" component. This PR exposes position as css variables, so it's easy to shift the button to the side or above any other bottom right content.
    image

  2. Subjective opinion: it is too visually prominent, I propose switching it from color-primary to color-secondary. The entry animation, plus the use of color-primary really draws your attention to it on entrance. It is a minor convenience feature, and should not act as a distraction from reading content.

Additionally:

  • it was missing an ARIA label, no indication of what this button was for a screen reader
  • It lacked full themability:
    • icon should be in css to allow customization, removed svg, can re-use an existing icon even.
    • color shouldn't introduce a hardcode value, should use existing variable
    • buttons use color-dark for hover state rather than opacity, should match styling
    • should use box-shadow infima variable rather than custom shadow
    • posistion should be user themeable.

Have you read the Contributing Guidelines on pull requests?

Yes.

Test Plan

Button as now rendered in default theme
image

I am unsure whether I did the change to base.json correctly. Couldn't figure out what the convention for naming them or if there was any logic to order, hope this was right. I just added it to the end.

Related PRs

#5419

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Sep 2, 2021
@Josh-Cena
Copy link
Collaborator

Back to top button should actually be safe to swizzle, it's just because the whitelist hasn't been updated for long🤦‍♂️ cf #5263

@netlify
Copy link

netlify bot commented Sep 2, 2021

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: 1ac9a36

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/613c8395c067430009572630

😎 Browse the preview: https://deploy-preview-5469--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Sep 2, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 80
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-5469--docusaurus-2.netlify.app/

@slorber
Copy link
Collaborator

slorber commented Sep 2, 2021

I'll be on holiday soon, but @lex111 can probably review this PR

Some quick thought

  • we should mark it safe to swizzle
  • we should add a stable className (ThemeClassNames) file so that it's easy to target with custom CSS
  • adding new CSS vars would become useless
  • didn't we remove this aria-label on purpose @lex111 ?

@dsmmcken
Copy link
Contributor Author

dsmmcken commented Sep 2, 2021

  • we should add a stable className (ThemeClassNames) file so that it's easy to target with custom CSS

That would be great, I haven't seen that adopted yet, what are the naming conventions?

  • didn't we remove this aria-label on purpose @lex111 ?

Title was removed, but it really should have an aria-label still for a11y, otherwise this button is a complete mystery as to what it does.

Copy link
Contributor

@lex111 lex111 left a comment

Choose a reason for hiding this comment

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

Thanks for PR, but at the moment in custom CSS it is not possible to override CSS vars without using !important rule. Actually, I would prefer to use stable CSS class name instead of having bunch of CSS vars if users want to change default button style. aria-label is a really useful thing for a11y. Not sure about changing of background button , I'll have to think about that.

@dsmmcken
Copy link
Contributor Author

dsmmcken commented Sep 2, 2021

Oh interesting, I assumed because I can override --ifm variables, I could override --docusaurus variables. That seems like something I should be able to do. I see the inclusion order is different. Is there any ticket to improve this?

re: css vars vs stable class names. I would consider maintaining both. Having stuff pulled out as css variables make it easier for handling light/dark mode overrides, and it also kind of act as guide rails to users doing theming. It makes it clear, here are things you might need to customize, but if you need to do more than override the css directly.

Guidance for adding a stable class name? docusaurus-button-back-to-top?

@lex111
Copy link
Contributor

lex111 commented Sep 3, 2021

Yes, we have dedicated issue to track this #3678

Most likely minority of users will customize back-to-top button, so I don't want to add even more CSS vars (we have so many not very used ones already). So stable CSS class name seems like good compromise. This can be confusing, so we probably need to document more about CSS customization later.

For now suggest adding new class name here, such as theme-back-to-top. https://github.com/facebook/docusaurus/blob/main/packages/docusaurus-theme-common/src/utils/ThemeClassNames.ts#L35

About background, think you're right, at the moment the button is too noticeable. So we can use gray shade, by analogy with button-secondary button.

@lex111
Copy link
Contributor

lex111 commented Sep 7, 2021

@dsmmcken please let me know if you cannot finish this PR, then I will do it myself.

@dsmmcken
Copy link
Contributor Author

dsmmcken commented Sep 7, 2021

@lex111 Was away for the long weekend, I can update it now that I am back.

@dsmmcken
Copy link
Contributor Author

dsmmcken commented Sep 8, 2021

Ready for re-review :

@dsmmcken dsmmcken requested a review from lex111 September 8, 2021 02:41
Copy link
Contributor

@lex111 lex111 left a comment

Choose a reason for hiding this comment

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

Perhaps we need to add dark mode support also?

Current state:

image

@dsmmcken dsmmcken requested a review from lex111 September 8, 2021 13:48
@lex111 lex111 added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label Sep 11, 2021
@lex111 lex111 merged commit b6d0378 into facebook:main Sep 11, 2021
@lex111
Copy link
Contributor

lex111 commented Sep 11, 2021

@dsmmcken thanks!

@@ -96,7 +97,7 @@
"theme.docs.versions.latestVersionLinkLabel": "latest version",
"theme.docs.versions.latestVersionLinkLabel___DESCRIPTION": "The label used for the latest version suggestion link label",
"theme.docs.versions.latestVersionSuggestionLabel": "For up-to-date documentation, see the {latestVersionLink} ({versionLabel}).",
"theme.docs.versions.latestVersionSuggestionLabel___DESCRIPTION": "The label used to tell the user to check the latest version",
"theme.docs.versions.latestVersionSuggestionLabel___DESCRIPTION": "The label userd to tell the user that he's browsing an unmaintained doc version",
Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI this description change has a typo and will be overridden anyway, as it's extracted from the code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: polish This PR adds a very minor behavior improvement that users will enjoy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants