-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
refactor: Renders addSlice in SPA #20675
refactor: Renders addSlice in SPA #20675
Conversation
Codecov Report
@@ Coverage Diff @@
## master #20675 +/- ##
===========================================
- Coverage 66.85% 55.20% -11.66%
===========================================
Files 1753 1753
Lines 65839 65835 -4
Branches 7007 7007
===========================================
- Hits 44017 36344 -7673
- Misses 20036 27705 +7669
Partials 1786 1786
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
24a371e
to
9e22ac4
Compare
9e22ac4
to
614131a
Compare
<AddSliceContainer | ||
user={user} | ||
addSuccessToast={() => null} | ||
{...routeProps} |
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.
Nit: I think you can use MemoryRouter instead of manually adding routeProps
<MemoryRouter>
<AddSliceContainer ... />
</MemoryRouter>
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 won't satisfy Typescript 😞. We should probably change the tests to RTL in the future where we could use the helper that allows composition.
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.
Works perfectly! LGTM
SUMMARY
This PR eliminates another React entry point by integrating the add chart route to SPA. This will also reduce the number of requests to the server because this route only depends on
user
information which is already available in the initial bootstrap data.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Screen.Recording.2022-07-11.at.3.09.51.PM.mov
TESTING INSTRUCTIONS
See the video for instructions.
ADDITIONAL INFORMATION