-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat: Add in option to use Vite #3191
feat: Add in option to use Vite #3191
Conversation
…urrent versions are fine
dbcc580
to
47afd30
Compare
Bundle ReportChanges will increase total bundle size by 1.3kB ⬆️
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3191 +/- ##
==========================================
- Coverage 98.14% 98.11% -0.04%
==========================================
Files 935 936 +1
Lines 14510 14513 +3
Branches 3941 3886 -55
==========================================
- Hits 14241 14239 -2
- Misses 264 269 +5
Partials 5 5
... and 8 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3191 +/- ##
==========================================
- Coverage 98.14% 98.11% -0.04%
==========================================
Files 935 936 +1
Lines 14510 14513 +3
Branches 3886 3973 +87
==========================================
- Hits 14241 14239 -2
- Misses 264 269 +5
Partials 5 5
... and 8 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3191 +/- ##
==========================================
- Coverage 98.14% 98.11% -0.04%
==========================================
Files 935 936 +1
Lines 14510 14513 +3
Branches 3968 3886 -82
==========================================
- Hits 14241 14239 -2
- Misses 264 269 +5
Partials 5 5
... and 8 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #3191 +/- ##
================================================
- Coverage 98.14000 98.11000 -0.03000
================================================
Files 935 936 +1
Lines 14510 14513 +3
Branches 3968 3941 -27
================================================
- Hits 14241 14239 -2
- Misses 264 269 +5
Partials 5 5
... and 8 files with indirect coverage changes
Continue to review full report in Codecov by Sentry.
|
Bundle ReportChanges will increase total bundle size by 1.3kB (0.02%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
✅ Deploy preview for gazebo ready!Previews expire after 1 month automatically.
|
README.md
Outdated
|
||
`.env.local` | ||
|
||
``` | ||
PROXY_TO=https://stage-api.codecov.dev | ||
REACT_APP_MY_CUSTOM_VAR=foobar | ||
REACT_APP_BASE_URL=http://localhost | ||
REACT_APPMY_CUSTOM_VAR=foobar |
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.
why is this a thing lol
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.
whoops typo
scripts/icon-list.mjs
Outdated
@@ -0,0 +1,67 @@ | |||
export const enabledIcons = [ | |||
'flag', |
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.
can we alphabetize this list?
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.
thank you co-pilot
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.
I spent like 10mins trying to figure out why I had one less line when I used it to alphabetize it, turns out we had two branch
's
@@ -86,15 +86,16 @@ describe('Alert', () => { | |||
|
|||
describe('Custom Icons', () => { | |||
const alert = ( | |||
<Alert variant={AlertOptions.INFO} customIconName="sparkles"> | |||
<Alert variant={AlertOptions.INFO} customIconName="speakerphone"> |
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.
this seems a little random lol
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.
TS was failing to build with Vite, sparkles doesn't appear to be a valid icon name, not sure how it was happy before.
const tableRoles = await screen.findAllByRole('row') | ||
expect(tableRoles).toHaveLength(2) | ||
|
||
const tableCells = await within(tableRoles[1]!).findAllByRole('cell') |
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.
Okay I needed within
in my life and didn't even know it thanks
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.
it is super useful
import svgr from 'vite-plugin-svgr' | ||
|
||
export default defineConfig((config) => { | ||
const env = loadEnv(config.mode, process.cwd(), 'REACT_APP') |
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.
couldn't we do REACT_APP_
here?
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.
Either or doesn't matter
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.
Looks good! Am excited to try this.
.gitignore
Outdated
@@ -55,3 +55,8 @@ codecov | |||
# VSCode | |||
.vscode | |||
|
|||
# ignore icon index files as they're genreated when executing | |||
src/old_ui/Icon/svg/index.jsx |
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.
Does this have any effect on enabling new icons?
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.
This didn't actually work, i should take this out
Description
This PR adds in Vite to Gazebo, and allows you to use it at the same time as our current CRACO setup. This is so we can continue using CRACO for our production builds, and testing until we have everything fully moved over and feel comfortable with Vite.
To use Vite locally for development you can use the
yarn build:vite
command. As a workaround to how Vite handles icons differently whenever you run a command that is Vite related it will generate new icon index.jsx files. You can remove these by either running any of the base CRACO commands OR runningyarn generate-icons:webpack
. Once we have fully transitioned over this requirement will be removed.Another thing to note down is the way that Vite loads in
.env
files. We currently use.env.local
for private environment variables, however Vite will override these values with the ones located in.env.development
to resolve this issue, copy your.env.local
and rename it to.env.development.local
.Notable Changes
*.test.*
to*.spec.*