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

Feature: cra template overhaul #181

Merged
merged 10 commits into from
Jun 16, 2021
Merged

Feature: cra template overhaul #181

merged 10 commits into from
Jun 16, 2021

Conversation

mmv08
Copy link
Member

@mmv08 mmv08 commented Jun 7, 2021

This PR does an overhaul of our create-react-app template:

  • Closes Add test example for cra-template-safe-app #85 by adding a utils file for testing and a test example
  • gets rid of react-app-rewired which is not well maintained and we didn't really need it, it could be replaced via a hack with cra's setupProxy.js
  • Update dependencies for both sdks/template
  • Removes weird stuff/styles from template's App component
  • Updates typescript config for template
  • Remove prettier/pre-commit hooks from web3react/sdk package.jsons

Testing:

  1. run npx create-react-app safe-app-template --template file:./safe-apps-sdk/packages/cra-template-safe-app - The path assumes you are at the same directory as a safe-apps-sdk repo folder
  2. run yarn start and yarn test

@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2021

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2021

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@@ -2,14 +2,13 @@
// https://create-react-app.dev/docs/proxying-api-requests-in-development/#configuring-the-proxy-manually

module.exports = function (app) {
app.use("/manifest.json", function (req, res, next) {
app.use('/manifest.json', function (req, res, next) {
Copy link
Member

Choose a reason for hiding this comment

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

If we only have the cors proxy for the manifest can the icons be properly loaded?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. No, they can't. I'll just apply it for every route

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, thinking about it a little bit more I'm not sure. The icons can be properly loaded via HTML img tag. They cannot be fetched from js code, though.

The interface doesn't display it because it tries to fetch it first: https://github.com/gnosis/safe-react/blob/development/src/routes/safe/components/Apps/utils.ts#L266 and if it doesn't work it doesn't try to use the URL anymore and uses placeholder image

Copy link
Member

Choose a reason for hiding this comment

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

🤦 ... yeah not optimal not sure ...

Little bit different question shouldn't react app rewired also cover this?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not well maintained and already has some caveats safe-global/safe-react-apps#88

Copy link
Member Author

Choose a reason for hiding this comment

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

If we can stay with "react-scripts" it's better to stay with it, chances of getting updates are way higher

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense :)

@mmv08 mmv08 marked this pull request as ready for review June 7, 2021 11:41
@mmv08 mmv08 requested review from germartinez and katspaugh June 7, 2021 12:04
Copy link
Member

@katspaugh katspaugh left a comment

Choose a reason for hiding this comment

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

Great stuff! 🎉

<Button size="lg" color="primary" onClick={submitTx}>
Submit
</Button>
)}
Copy link
Member

Choose a reason for hiding this comment

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

This was kinda useful I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why? I think developers will remove it anyway and implement it themselves.

Copy link
Member

@katspaugh katspaugh Jun 9, 2021

Choose a reason for hiding this comment

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

I used it in Drain Safe. A good UX pattern. But yeah, it's pretty trivial to reimplement.

@changeset-bot
Copy link

changeset-bot bot commented Jun 16, 2021

⚠️ No Changeset found

Latest commit: 667970e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@mmv08 mmv08 merged commit c010671 into development Jun 16, 2021
@mmv08 mmv08 deleted the feature/cra-example-test branch June 16, 2021 09:24
@github-actions github-actions bot locked and limited conversation to collaborators Jun 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants