feat: split ConfirmPage storybook pages by transaction type #25283
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
| history.replace. To avoid history.replace, we can provide a param id. */} | ||
| <MemoryRouter initialEntries={['/confirmation/:0']}> | ||
| <Route path="/confirmation/:id" render={() => <ConfirmPage />} /> | ||
| </MemoryRouter> |
There was a problem hiding this comment.
Without the Router workaround, we would receive an error like this one
which broke the bundler for subsequent storybook loads
ui/pages/confirmations/confirm/stories/signatures/permit.stories.tsx
Outdated
Show resolved
Hide resolved
| * The `<ConfirmPage>` that's displayed when the current confirmation is a version "V1" `eth_signTypedData` signature. | ||
| */ | ||
| export default { | ||
| title: 'Pages/Confirmation/ConfirmPage/Signatures/SignTypedDataV1', |
There was a problem hiding this comment.
Splitting these into separate files makes total sense, but is there a limit to how far we should go?
Arguably Permit could go into V3 and V4 for example since it's a subset of that, and the same with SIWE and personal sign but I assume the argument here is we present them as distinct confirmations hence the separate file.
This makes sense to me, just confirming the logic.
There was a problem hiding this comment.
By distinct confirmations was the initial idea, though with this second thought, distinct confirmations might be tricky to pin since we have layers of distinctions.
Great callout! updated b19224b merging subtypes. We could revert this if we do find it more helpful to later
| {/* Adding the MemoryRouter and Route is a workaround to bypass a 404 error in storybook that | ||
| is caused when the 'ui/pages/confirmations/hooks/syncConfirmPath.ts' hook calls | ||
| history.replace. To avoid history.replace, we can provide a param id. */} | ||
| <MemoryRouter initialEntries={['/confirmation/:0']}> |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #25283 +/- ##
========================================
Coverage 65.39% 65.39%
========================================
Files 1382 1382
Lines 54764 54764
Branches 14369 14369
========================================
Hits 35809 35809
Misses 18955 18955 ☔ View full report in Codecov by Sentry. |
Builds ready [ab6f248]
Page Load Metrics (51 ± 3 ms)
Bundle size diffs
|
metamaskbot
added
the
release-12.1.0
Description
Related issues
Fixes: #25052 (continuation)
Related: #25054
Manual testing steps
yarn storybookScreenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist