-
Notifications
You must be signed in to change notification settings - Fork 369
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
upgrade react scripts #507
Conversation
✅ Deploy Preview for vigilant-albattani-df38ec ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for superb-tarsier-e2aa29 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Deploying with
|
Latest commit: |
19c65c5
|
Status: | ✅ Deploy successful! |
Preview URL: | https://b2eb427f.gmx-interface.pages.dev |
Branch Preview URL: | https://upgrade-cra.gmx-interface.pages.dev |
@@ -0,0 +1 @@ | |||
GENERATE_SOURCEMAP=false |
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 it mean local server and/or test stands won't have source maps?
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 is set for production.
From docs:
When set to false, source maps are not generated for a production build. This solves out of memory (OOM) issues on some smaller machines.
https://create-react-app.dev/docs/advanced-configuration/
Active PR for the issue: facebook/create-react-app#11752
src/index.d.ts
Outdated
@@ -0,0 +1 @@ | |||
// # |
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.
do we need this?
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 was getting an error and it was suggested to have a file named index.d.ts. So I added that. I will look into the issue properly.
@@ -22,8 +22,7 @@ function getFeeLabel(type: FeeType) { | |||
deposit: t`Deposit Fee`, | |||
execution: t`Execution Fee`, | |||
}; | |||
|
|||
return i18n._(labels[type]); | |||
return i18n._(/*i18n*/ labels[type]); |
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.
mm, is /i18n/ some kind of magic comment for lingui? Or can it be deleted?
Btw, probably this syntax is not needed as getFeeLabel is called inside component during render so language should update correctly with simple return?
return labels[type]
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.
Yeah, I tested it and works well. I have removed it now.
src/components/Footer/constants.ts
Outdated
{ text: defineMessage({ message: "Terms and Conditions" }), link: "/terms-and-conditions" }, | ||
{ text: defineMessage({ message: "Referral Terms" }), link: "/referral-terms" }, | ||
{ text: defineMessage({ message: "Media Kit" }), link: "https://gmxio.gitbook.io/gmx/media-kit", external: true }, | ||
{ text: t`Terms and Conditions`, link: "/terms-and-conditions" }, |
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.
but here FOOTER_LINKS should be inside getFooterLinks function?
Otherwise they are only initialized once?
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.
Moved it. Thanks for noticing.
[x] Upgrade react-scripts to v5