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

lazy load NotificationBar #4278

Merged
merged 9 commits into from Feb 20, 2021
Merged

Conversation

@chenxsan
Copy link
Member

@chenxsan chenxsan commented Dec 8, 2020

closes #4414

Currently the NotificationBar is included in precompiled HTML, which means browsers would render it before we hide it with js which caused a flash of unstyled content.

In this pull request,

  1. we lazy load it which means it's eliminated from the precompiled HTML
  2. we hide it by default so returning users who had it hidden won't see it at all, and they don't have to load the extra script as well, only new users would see a flash of unstyled content at the top. We can improve this for new users by moving the notification bar to the right bottom when we have the UPDATE button removed.

Note that we'll fix the following issue in another pull request in near future.

We have people complaining on twitter that the snippets of webpack search results show content of notification bar.

image

I've inspected the search results and did find something like below which is bad for user experience:

image

This pull request would lazy load the notification bar and stop it from included in html.

@vercel
Copy link

@vercel vercel bot commented Dec 8, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/webpack-docs/webpack-js-org/gp72bh88q
Preview: https://webpack-js-org-git-fork-chenxsan-feature-lazy-load-notification.webpack-docs.vercel.app

@chenxsan
Copy link
Member Author

@chenxsan chenxsan commented Dec 8, 2020

This method just works around the issue, not fix it. We might need something like react-helmet to specify <meta name='description' /> for every page.

@jeffin143
Copy link
Contributor

@jeffin143 jeffin143 commented Dec 10, 2020

Although workaround, But I really like the improvement , and the thought 🚀

@montogeek montogeek added the UI/UX label Jan 5, 2021
@montogeek
Copy link
Member

@montogeek montogeek commented Jan 5, 2021

I would also prefer a proper solution, and return the correct HTML tags from the server.

chenxsan added 2 commits Jan 14, 2021
@chenxsan chenxsan force-pushed the chenxsan:feature/lazy-load-notification branch from e6696a5 to ed2150a Feb 18, 2021
@chenxsan chenxsan marked this pull request as ready for review Feb 19, 2021
@chenxsan chenxsan merged commit 039b0d1 into webpack:master Feb 20, 2021
6 checks passed
6 checks passed
Lint (ubuntu-latest, 12.x)
Details
Proselint (ubuntu-latest) Proselint (ubuntu-latest)
Details
Check Links (ubuntu-latest, 12.x)
Details
End to End Testing (ubuntu-latest, 12.x)
Details
Vercel Deployment has completed
Details
licence/cla Contributor License Agreement is signed.
Details
@chenxsan chenxsan deleted the chenxsan:feature/lazy-load-notification branch Feb 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants