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 #16979

Merged
merged 8 commits into from Mar 26, 2023
Merged

docs: add back to top button #16979

merged 8 commits into from Mar 26, 2023

Conversation

Tanujkanti4441
Copy link
Contributor

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[x] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Added a back to top button on docs pages for below 1400px screen size and gave it a classname 'scroll-up-btn', added the scroll up button related JS (made a new file) and CSS.

for light mode

Screenshot 2023-03-12 144538

for dark mode

Screenshot 2023-03-12 144603

#### Is there anything you'd like reviewers to focus on? PR is related to the issue #16932.

@Tanujkanti4441 Tanujkanti4441 requested a review from a team as a code owner March 12, 2023 09:20
@eslint-github-bot eslint-github-bot bot added triage An ESLint team member will look at this issue soon documentation Relates to ESLint's documentation labels Mar 12, 2023
@netlify
Copy link

netlify bot commented Mar 12, 2023

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit 5aa9eab
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/6419503b11572800082fbacf
😎 Deploy Preview https://deploy-preview-16979--docs-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@snitin315 snitin315 added accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Mar 12, 2023
Copy link
Member

@kecrily kecrily 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 the accessibility in light mode is not good enough, can you improve it? Like modifying the color of ↑.

@Tanujkanti4441
Copy link
Contributor Author

Tanujkanti4441 commented Mar 14, 2023

I think the accessibility in light mode is not good enough, can you improve it? Like modifying the color of ↑.

Done.

@harish-sethuraman harish-sethuraman linked an issue Mar 14, 2023 that may be closed by this pull request
1 task
@harish-sethuraman
Copy link
Member

Can you add a sample page url in PR where I can see scroll to top 🤔 Im not able to see it?

@Tanujkanti4441
Copy link
Contributor Author

Tanujkanti4441 commented Mar 14, 2023

Can you add a sample page url in PR where I can see scroll to top 🤔 Im not able to see it?

I think button is on every page of docs just need to scroll down to make it appear.
https://deploy-preview-16979--docs-eslint.netlify.app/rules/#deprecated

@amareshsm
Copy link
Member

Can't we achieve this same behavior without using js?

I think we achieve this using CSS which works well in no-js mode (when js is disabled):
positioning the scroll to the top button - position: sticky
for checking device-width - media query
scrolling to the top - a link that targets an ID on the <html> element.

Thoughts?

@kecrily
Copy link
Member

kecrily commented Mar 15, 2023

Can't we achieve this same behavior without using js?

I think we achieve this using CSS which works well in no-js mode (when js is disabled):

positioning the scroll to the top button - position: sticky

for checking device-width - media query

scrolling to the top - a link that targets an ID on the <html> element.

Thoughts?

Yes, I think no-js support is so cool. And it is true that we can achieve this feature without js.

@Tanujkanti4441
Copy link
Contributor Author

Can't we achieve this same behavior without using js?

I think we achieve this using CSS which works well in no-js mode (when js is disabled): positioning the scroll to the top button - position: sticky for checking device-width - media query scrolling to the top - a link that targets an ID on the <html> element.

Thoughts?

Agree with this, but i am not sure about how Position: sticky will help the button to achieve the same behavior (appear after scrolling) because for that, button should be placed on some where below from the bottom of the view-port and as i tried position: sticky and not able to move it any where from its default position (bottom of the page and aligned to the left side).
is there something we can do or am i missing something.

@harish-sethuraman
Copy link
Member

May be we can remove the sticky one when JS is enabled after scrolling a bit. If not lets leave it as such. Also for scrolling to top lets use id based approach without any JS.

@Tanujkanti4441
Copy link
Contributor Author

May be we can remove the sticky one when JS is enabled after scrolling a bit. If not lets leave it as such. Also for scrolling to top lets use id based approach without any JS.

Done! The back to button is now functioning without js.

Copy link
Member

@harish-sethuraman harish-sethuraman left a comment

Choose a reason for hiding this comment

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

Lets make sure we add display: none for this element in the print.scss so that we dont see the arrow in print preview :). Other than that everything looks perfect thanks!

Copy link
Member

@harish-sethuraman harish-sethuraman left a comment

Choose a reason for hiding this comment

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

LGTM.
cc: @eslint/website-team friendly ping for review

Copy link
Member

@kecrily kecrily left a comment

Choose a reason for hiding this comment

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

LGTM

@kecrily kecrily requested a review from snitin315 March 23, 2023 11:31
@nzakas
Copy link
Member

nzakas commented Mar 24, 2023

@kecrily just a reminder: if you approve a PR and don't merge it, please explain why. Right now I'm not sure if you're expecting something else to happen or not.

(function () {
const scrollUpBtn = document.getElementById("scroll-up-btn");

if(window.innerWidth < 1400) {
Copy link
Member

Choose a reason for hiding this comment

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

I have a suggestion here. This width check can be removed and media query can be used instead to show or hide scroll button for desktop. In no-js right now it will show in desktop device too?

@kecrily
Copy link
Member

kecrily commented Mar 25, 2023

@kecrily just a reminder: if you approve a PR and don't merge it, please explain why. Right now I'm not sure if you're expecting something else to happen or not.

@nzakas I thought that my request for a review of snitin315 had shown the reason why I didn't merge it. But it doesn't matter. I will indicate this separately next time.

@@ -154,6 +154,7 @@
<script src="{{ '/assets/js/focus-visible.js' | url }}"></script>
<script src="{{ '/assets/js/main.js' | url }}"></script>
<script src="{{ '/assets/js/tabs.js' | url }}"></script>
<script src="{{ '/assets/js/scroll-up-btn.js' | url }}"></script>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<script src="{{ '/assets/js/scroll-up-btn.js' | url }}"></script>

Comment on lines +105 to +107
<a id="scroll-up-btn" href="#site_top">
<svg fill="none" height="24" stroke="currentColor" stroke-linecap="round" stroke-linejoin="round" stroke-width="2" viewBox="0 0 24 24" width="24"><line x1="12" x2="12" y1="19" y2="5"/><polyline points="5 12 12 5 19 12"/></svg>
</a>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<a id="scroll-up-btn" href="#site_top">
<svg fill="none" height="24" stroke="currentColor" stroke-linecap="round" stroke-linejoin="round" stroke-width="2" viewBox="0 0 24 24" width="24"><line x1="12" x2="12" y1="19" y2="5"/><polyline points="5 12 12 5 19 12"/></svg>
</a>
<div class="scroll-up-container">
<a id="scroll-up-btn" href="#site_top">
<svg fill="none" height="24" stroke="currentColor" stroke-linecap="round" stroke-linejoin="round" stroke-width="2" viewBox="0 0 24 24" width="24"><line x1="12" x2="12" y1="19" y2="5"/><polyline points="5 12 12 5 19 12"/></svg>
</a>
</div>

Comment on lines +1 to +13
(function () {
const scrollUpBtn = document.getElementById("scroll-up-btn");

if(window.innerWidth < 1400) {
window.addEventListener("scroll", function () {
if(document.body.scrollTop > 500 || document.documentElement.scrollTop > 500) {
scrollUpBtn.style.display = "flex";
} else {
scrollUpBtn.style.display = "none";
}
});
}
})();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(function () {
const scrollUpBtn = document.getElementById("scroll-up-btn");
if(window.innerWidth < 1400) {
window.addEventListener("scroll", function () {
if(document.body.scrollTop > 500 || document.documentElement.scrollTop > 500) {
scrollUpBtn.style.display = "flex";
} else {
scrollUpBtn.style.display = "none";
}
});
}
})();

@@ -207,3 +207,7 @@ ul {
margin: 1cm;
}
}

#scroll-up-btn {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#scroll-up-btn {
.scroll-up-container {

Comment on lines +160 to +183

#scroll-up-btn {
width: 50px;
height: 50px;
display: none;
position: fixed;
right: 50px;
bottom: 35px;
font-size: 1.5rem;
border-radius: 50%;
color: var(--body-background-color);
text-decoration: none;
justify-content: center;
align-items: center;
background-color: var(--link-color);

@media (max-width: 800px) {
right: 35px;
}

@media (max-width: 600px) {
right: 25px;
}
}
Copy link
Member

@amareshsm amareshsm Mar 25, 2023

Choose a reason for hiding this comment

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

Suggested change
#scroll-up-btn {
width: 50px;
height: 50px;
display: none;
position: fixed;
right: 50px;
bottom: 35px;
font-size: 1.5rem;
border-radius: 50%;
color: var(--body-background-color);
text-decoration: none;
justify-content: center;
align-items: center;
background-color: var(--link-color);
@media (max-width: 800px) {
right: 35px;
}
@media (max-width: 600px) {
right: 25px;
}
}
.scroll-up-container {
display: none;
position: absolute;
top: 0;
right: 30px;
height: 100%;
&::before {
content: "";
display: block;
height: 120vh;
pointer-events: none;
}
#scroll-up-btn {
position: sticky;
top: 80vh;
cursor: pointer;
width: 50px;
height: 50px;
display: flex;
font-size: 1.5rem;
border-radius: 50%;
color: var(--body-background-color);
text-decoration: none;
justify-content: center;
align-items: center;
background-color: var(--link-color);
transition: all 1s ease-in;
}
}
@media screen and (max-width: 1400px) {
.scroll-up-container {
display: block;
}
}

@amareshsm
Copy link
Member

I tried to achieve the same behavior without using js. Pls have a look. I checked locally It works.

@Tanujkanti4441
Copy link
Contributor Author

I tried to achieve the same behavior without using js. Pls have a look. I checked locally It works.

@amareshsm i tried it as the same way as yours and it is working fine on desktop but not on mobile/tablet because in browser apps, on scrolling, the viewable screen height is also changing because browsers widget (like for changing the tabs and URL input) is disappearing on scrolling up and appearing on scrolling down,
and this behavior is affecting the position of back to top button.
is it same for you as well ?
if yes can we do something for this.

Screenshot

Screenshot_2023-03-25-16-57-46-01

Screenshot

Screenshot_2023-03-25-16-57-17-90

@amareshsm
Copy link
Member

I tried to achieve the same behavior without using js. Pls have a look. I checked locally It works.

@amareshsm i tried it as the same way as yours and it is working fine on desktop but not on mobile/tablet because in browser apps, on scrolling, the viewable screen height is also changing because browsers widget (like for changing the tabs and URL input) is disappearing on scrolling up and appearing on scrolling down, and this behavior is affecting the position of back to top button. is it same for you as well ? if yes can we do something for this.

Screenshot
Screenshot

Yes it is happening based on the device viewport height it is changing. We can write media queries based on height and handle it. For now we can go ahead with current approach.

Copy link
Member

@amareshsm amareshsm left a comment

Choose a reason for hiding this comment

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

LGTM. Waiting for code owner review.

Copy link
Contributor

@snitin315 snitin315 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for contributing.

@snitin315 snitin315 merged commit e39f28d into eslint:main Mar 26, 2023
21 checks passed
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Mar 29, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.36.0` -> `8.37.0`](https://renovatebot.com/diffs/npm/eslint/8.36.0/8.37.0) |

---

### Release Notes

<details>
<summary>eslint/eslint</summary>

### [`v8.37.0`](https://github.com/eslint/eslint/releases/tag/v8.37.0)

[Compare Source](eslint/eslint@v8.36.0...v8.37.0)

#### Features

-   [`b6ab8b2`](eslint/eslint@b6ab8b2) feat: `require-unicode-regexp` add suggestions ([#&#8203;17007](eslint/eslint#17007)) (Josh Goldberg)
-   [`10022b1`](eslint/eslint@10022b1) feat: Copy getScope() to SourceCode ([#&#8203;17004](eslint/eslint#17004)) (Nicholas C. Zakas)
-   [`1665c02`](eslint/eslint@1665c02) feat: Use plugin metadata for flat config serialization ([#&#8203;16992](eslint/eslint#16992)) (Nicholas C. Zakas)
-   [`b3634f6`](eslint/eslint@b3634f6) feat: docs license ([#&#8203;17010](eslint/eslint#17010)) (Samuel Roldan)
-   [`892e6e5`](eslint/eslint@892e6e5) feat: languageOptions.parser must be an object. ([#&#8203;16985](eslint/eslint#16985)) (Nicholas C. Zakas)

#### Bug Fixes

-   [`619f3fd`](eslint/eslint@619f3fd) fix: correctly handle `null` default config in `RuleTester` ([#&#8203;17023](eslint/eslint#17023)) (Brad Zacher)
-   [`1fbf118`](eslint/eslint@1fbf118) fix: `getFirstToken`/`getLastToken` on comment-only node ([#&#8203;16889](eslint/eslint#16889)) (Francesco Trotta)
-   [`129e252`](eslint/eslint@129e252) fix: Fix typo in `logical-assignment-operators` rule description ([#&#8203;17000](eslint/eslint#17000)) (Francesco Trotta)

#### Documentation

-   [`75339df`](eslint/eslint@75339df) docs: fix typos and missing info in id-match docs ([#&#8203;17029](eslint/eslint#17029)) (Ed Lucas)
-   [`ec2d830`](eslint/eslint@ec2d830) docs: Fix typos in the `semi` rule docs ([#&#8203;17012](eslint/eslint#17012)) (Andrii Lundiak)
-   [`e39f28d`](eslint/eslint@e39f28d) docs: add back to top button ([#&#8203;16979](eslint/eslint#16979)) (Tanuj Kanti)
-   [`721c717`](eslint/eslint@721c717) docs: Custom Processors cleanup and expansion ([#&#8203;16838](eslint/eslint#16838)) (Ben Perlmutter)
-   [`d049f97`](eslint/eslint@d049f97) docs: 'How ESLint is Maintained' page ([#&#8203;16961](eslint/eslint#16961)) (Ben Perlmutter)
-   [`5251a92`](eslint/eslint@5251a92) docs: Describe guard options for guard-for-in ([#&#8203;16986](eslint/eslint#16986)) (alope107)
-   [`6157d81`](eslint/eslint@6157d81) docs: Add example to guard-for-in docs. ([#&#8203;16983](eslint/eslint#16983)) (alope107)
-   [`fd47998`](eslint/eslint@fd47998) docs: update `Array.prototype.toSorted` specification link ([#&#8203;16982](eslint/eslint#16982)) (Milos Djermanovic)
-   [`3e1cf6b`](eslint/eslint@3e1cf6b) docs: Copy edits on Maintain ESLint docs ([#&#8203;16939](eslint/eslint#16939)) (Ben Perlmutter)

#### Chores

-   [`c67f299`](eslint/eslint@c67f299) chore: upgrade [@&#8203;eslint/js](https://github.com/eslint/js)[@&#8203;8](https://github.com/8).37.0 ([#&#8203;17033](eslint/eslint#17033)) (Milos Djermanovic)
-   [`ee9ddbd`](eslint/eslint@ee9ddbd) chore: package.json update for [@&#8203;eslint/js](https://github.com/eslint/js) release (ESLint Jenkins)
-   [`dddb475`](eslint/eslint@dddb475) chore: upgrade [@&#8203;eslint/eslintrc](https://github.com/eslint/eslintrc)[@&#8203;2](https://github.com/2).0.2 ([#&#8203;17032](eslint/eslint#17032)) (Milos Djermanovic)
-   [`522431e`](eslint/eslint@522431e) chore: upgrade espree@9.5.1 ([#&#8203;17031](eslint/eslint#17031)) (Milos Djermanovic)
-   [`f5f9a88`](eslint/eslint@f5f9a88) chore: upgrade eslint-visitor-keys@3.4.0 ([#&#8203;17030](eslint/eslint#17030)) (Milos Djermanovic)
-   [`4dd8d52`](eslint/eslint@4dd8d52) ci: bump actions/stale from 7 to 8 ([#&#8203;17026](eslint/eslint#17026)) (dependabot\[bot])
-   [`ad9dd6a`](eslint/eslint@ad9dd6a) chore: remove duplicate scss, ([#&#8203;17005](eslint/eslint#17005)) (Strek)
-   [`ada6a3e`](eslint/eslint@ada6a3e) ci: unpin Node 19 ([#&#8203;16993](eslint/eslint#16993)) (Milos Djermanovic)
-   [`c3da975`](eslint/eslint@c3da975) chore: Remove triage label from template ([#&#8203;16990](eslint/eslint#16990)) (Nicholas C. Zakas)
-   [`69bc0e2`](eslint/eslint@69bc0e2) ci: pin Node 19 to 19.7.0 ([#&#8203;16987](eslint/eslint#16987)) (Milos Djermanovic)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4yNC41IiwidXBkYXRlZEluVmVyIjoiMzUuMjQuNSJ9-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1834
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@Tanujkanti4441 Tanujkanti4441 deleted the scroll-up branch July 27, 2023 11:36
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Sep 23, 2023
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion contributor pool documentation Relates to ESLint's documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add a back to top button on docs page
6 participants