-
Notifications
You must be signed in to change notification settings - Fork 129
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
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
72643c1
Dependency bump
mmv08 5129767
split deps
mmv08 9f379ca
use proxy hack for cors
mmv08 61663e5
add test example
mmv08 80c5ecf
use jsdom env in sdk testing
mmv08 16d9b0d
prettier fixes
mmv08 23c010b
add return result
mmv08 0fa6125
move src folder
mmv08 3dfd76c
template updates, add prettier, eslint
mmv08 667970e
Merge branch 'development' of github.com:gnosis/safe-apps-sdk into fe…
mmv08 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Oops, something went wrong.
This file was deleted.
Oops, something went wrong.
This file was deleted.
Oops, something went wrong.
This file was deleted.
Oops, something went wrong.
This file was deleted.
Oops, something went wrong.
This file was deleted.
Oops, something went wrong.
This file was deleted.
Oops, something went wrong.
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,8 @@ | ||
import { render, screen } from "@testing-library/react" | ||
import App from "./App" | ||
import { render, screen } from './tests/utils'; | ||
import App from './App'; | ||
|
||
test("renders learn loading screen", () => { | ||
render(<App />) | ||
const waitingHeading = screen.getByText(/Waiting for Safe/i) | ||
expect(waitingHeading).toBeInTheDocument() | ||
}) | ||
test('renders learn loading screen', () => { | ||
render(<App />); | ||
const waitingHeading = screen.getByText(/Waiting for Safe/i); | ||
expect(waitingHeading).toBeInTheDocument(); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
File renamed without changes.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
If we only have the cors proxy for the manifest can the icons be properly loaded?
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.
Good catch. No, they can't. I'll just apply it for every route
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.
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
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 not optimal not sure ...
Little bit different question shouldn't react app rewired also cover 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.
It's not well maintained and already has some caveats safe-global/safe-react-apps#88
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.
If we can stay with "react-scripts" it's better to stay with it, chances of getting updates are way higher
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.
Makes sense :)