-
Notifications
You must be signed in to change notification settings - Fork 37
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
Switch to Vite from Create React App #1152
Conversation
So long as you don't look too closely...
Need to evaluate module vs classic
Import the module namespace object
These versions are cjs compatible
There seems to be some incompatibility, one we can return to when we're focussed on bundle size.
We got it working locally but not in the CI build
The Vite way seems to be to commit defaults in .env and use .env.local for uncommitted changes. It's already committed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM, left a few comments. Will leave for Rob to review
A few comments, but otherwise, the code changes look good and the app behaves as expected. |
Co-authored-by: Robert Knight <[email protected]> Co-authored-by: Grace <[email protected]>
I didn't expect these... will try to understand.
That's very odd. We do have some media-query-dependent styling in DocumentationTopLevelItem. |
This isn't language-change specific. It happens on load too. |
Otherwise we needlessly render assuming false which causes rerenders with visual change at start-up and on language change. Not Vite related, we need to rescue this if we abandon the PR.
Fixed in d4313dd. I think this was a Chakra UI upgrade issue rather than a Vite one. |
I've so far failed to reproduce the error in @microbit-robert's screenshot. I do see this though, for example when starting in Dutch and switching to English (but not the other way around).
|
Previously we pulled the language off what we indexed which could cause an issue when the search index was updated for a new language but the content had not been reloaded. Instead an index now knows its language. Additionally, ensure we wait for the languages of all content to match before indexing.
Fixed in 994c4a7. Whether you could reproduce this depended on timing but was pretty easy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
npm audit issues are now at 0 🎉
Notable changes:
Tasks: