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.

Sign up for GitHub

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

Jump to bottom

chore: reduce cumulative layout shifts #15926

Closed
wants to merge 8 commits into from
Closed

chore: reduce cumulative layout shifts #15926

wants to merge 8 commits into from

Conversation

amareshsm
Copy link
Member

@amareshsm amareshsm commented May 26, 2022 •
edited

Prerequisites checklist

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

[ ] 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
[x] Other, please explain:
Reduced CLS in new docs site.

What changes did you make? (Give an overview)

Reduced CLS.

@eslint-github-bot eslint-github-bot bot added triage An ESLint team member will look at this issue soon chore This change is not user-facing labels May 26, 2022
@netlify
Copy link

netlify bot commented May 26, 2022 •
edited

✅ Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit 93ecd0e
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/62937c540f9e39000996e0b3
😎 Deploy Preview https://deploy-preview-15926--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.

@mdjermanovic
Copy link
Member

Edit: Lighthouse reports for desktop version

What's the result for mobile? I'm getting CLS 0.9 on new.eslint.org/docs

@mdjermanovic
Copy link
Member

This is what I'm getting locally with the latest main, for the docs homepage:

Desktop:

image

Mobile:

image

@amareshsm
Copy link
Member Author

Please pull the latest commit. I tried using the <noscript> tag.

@amareshsm
Copy link
Member Author

amareshsm commented May 27, 2022 •
edited

This is what I'm getting locally with the latest main, for the docs homepage:

Desktop:

image

Mobile:

image

Exactly the same results I am getting in the main branch.

After changes (in reduce-layout-shifts branch):

URL - http://localhost:2023/docs/

Desktop:
image

Mobile:
image

PS: I haven't removed unused/repeated code and haven't focused on naming conventions. I would like to know your thoughts on this approach.

mdjermanovic
mdjermanovic reviewed May 27, 2022
View reviewed changes
docs/src/_includes/components/docs-index.html Outdated Show resolved Hide resolved
docs/src/_includes/components/docs-index.html Outdated Show resolved Hide resolved
docs/src/_includes/components/docs-index.html Outdated Show resolved Hide resolved
@mdjermanovic
Copy link
Member

mdjermanovic commented May 27, 2022 •
edited

Now I noticed we have class="no-js" set on the html tag, and a script that replaces it with enhanced, so that seems to be the intended way to do this.

Can you try with that instead of noscript? (e.g., .no-js #js-docs-index-list { display: block !important }).

.enhanced could hide the fallback links.

I think we should remove type = "module" from the script because modules are deferred, while we probably need that code to execute as soon as possible.

mdjermanovic
mdjermanovic reviewed May 28, 2022
View reviewed changes
docs/src/_includes/components/nav-version-switcher.html Outdated
Comment on lines 2 to 4
<noscript>
<a href="/versions/" class="switcher-fallback">Versions</a>
</noscript>
Copy link
Member

Choose a reason for hiding this comment

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

Could we use the enhanced class name (the one that replaces no-js on html) to hide all fallback links, and then we won't need to use <noscript>?

Something like .enhanced .switcher-fallback { display: none !important } in the <style> we added to <head>.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, we can use the enhanced class instead of <noscript>.

mdjermanovic
mdjermanovic approved these changes May 30, 2022
View reviewed changes
Copy link
Member

@mdjermanovic mdjermanovic 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!

This looks noticeably better than before, especially on mobile. And the lighthouse report shows the improvement:

CLS before: 0.9
CLS after: < 0.1

I would like more reviews from other team members before merging.

@mdjermanovic mdjermanovic marked this pull request as draft May 30, 2022 23:05
@mdjermanovic
Copy link
Member

I converted this to draft since we first need to sync these files with the latest version of the prototype (#15944), as we noticed that the initial version in this repo was not the latest one from the prototype.

@amareshsm
Copy link
Member Author

Closing this in favour of #16047

@amareshsm amareshsm closed this Jun 23, 2022
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Dec 21, 2022
@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 Dec 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mdjermanovic mdjermanovic mdjermanovic approved these changes

Assignees
No one assigned
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion chore This change is not user-facing triage An ESLint team member will look at this issue soon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@amareshsm @mdjermanovic