Extract out confirm-data and confirm-hex-data components from confirm-transaction-base.component.js #17822
Conversation
…-tarnsaction-base component
|
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. |
…k-extension into confirm_data_components
Builds ready [679c24c]
Page Load Metrics (1410 ± 30 ms)
Bundle size diffs
|
Builds ready [4c98fe2]
Page Load Metrics (1514 ± 53 ms)
Bundle size diffs
|
…k-extension into confirm_data_components
…k-extension into confirm_data_components
…k-extension into confirm_data_components
Hey @georgewrmarshall : this component has dependency on app state, I am finding it hard to write stories. |
Builds ready [95a02f9]
Page Load Metrics (1519 ± 36 ms)
Bundle size diffs
|
There was a problem hiding this comment.
as mentioned here https://github.com/MetaMask/metamask-extension/pull/17822/files#r1117481915, I think it makes more sense to keep these components in ui/pages/confirm-transaction-base rather than ui/components/app since they seem to only be used in the ConfirmTransactionBase component and they aren't planned to be used elsewhere. Maybe we can get a 3rd person to weigh in on this.
If we do keep them in ui/components/app I think it's a good idea to have storybook pages associated with them
|
PR is updated with storybooks. |
Builds ready [6e53ff1]
Page Load Metrics (1766 ± 62 ms)
Bundle size diffs
|
There was a problem hiding this comment.
LGTM!
spoke about keeping the components in ui/components/app with @jpuri & @Gudahtt. We're okay moving forward with this
[maybe non-blocking question] cc: @jpuri @brad-decker @georgewrmarshall
In regards to #17822 (comment)
There are a couple you have targeted to be rendered as h3 tags but with bodySm styling, is that intentional?
I'm a little confused by this too. Does anyone here happen to know:
- do we have plans to add styles for heading tags (e.g.
<h1>or<h3>)? - do we have plans to add additional TextVariant styles for heading tags so we don't use other non-heading TextVariants like
bodySm? else, is this a normal use-case to combine heading tags with non-heading TextVariants?
There was a problem hiding this comment.
Great work on the stories! I'm wondering if they could be improved further by providing the mock state to display more visual states? It may not be possible but have added a way to change the display of a component using mocked state. Some other minor suggestions. Also great work on usage of design system components and reducing CSS 💯
| DataComponentStory.args = { | ||
| txData, | ||
| dataComponent, | ||
| }; |
There was a problem hiding this comment.
Fantastic work! Is there a state where this component shows more than the error state? If it requires different redux stat you can add that by providing a provider
import React from 'react';
import { Provider } from 'react-redux';
import testData from '../../../../.storybook/test-data'; // this is the test data that you can use to populate the store
import configureStore from '../../../store/store';
import ConfirmData from './confirm-data';
const store = configureStore(testData); // this is the store that you can use to wrap the component in you could customize it with the specific data you need per story
export default {
title: 'Components/App/ConfirmData',
decorators: [(story) => <Provider store={store}>{story()}</Provider>], // if you need to wrap the component in a provider, you can do it here
argTypes: {
txData: {
control: 'object',
},
dataComponent: {
control: 'element',
},
},
args: {
txData: {
txParams: {
data: '0x608060405234801',
},
origin: 'https://metamask.github.io/',
type: 'transfer',
},
},
};
export const DefaultStory = (args) => <ConfirmData {...args} />;
DefaultStory.storyName = 'Default';
export const NoData = (args) => <ConfirmData {...args} />;
ConfirmData.args = {
txData: {},
};
export const DataComponent = (args) => <ConfirmData {...args} />;
ConfirmData.args = {
dataComponent: <span>Data Component</span>,
};
There was a problem hiding this comment.
I found it showing this message for most of transactions. It requires some api query to get these insights and some other state combination to get insights - I am not very sure about that.
There was a problem hiding this comment.
Ah ok. TBH I'm still not sure how we can do that. I'll do some research. I think there's still a huge benefit to showing the error state though 💯 Thanks @jpuri 🙏
| : ''; | ||
|
|
||
| return ( | ||
| <Box padding={4}> |
There was a problem hiding this comment.
I would consider this assumed layout which is adding spacing to a component before knowing where it might be placed? I'm not sure where we are on spreading props but if you were to spread props here it would allow the padding and spacing to be changed at the implementation stage e.g.
<Box {...props}
Then on the implementation side
<ConfirmHexData txParams={txParams} padding={4} marginTop={4} />
There was a problem hiding this comment.
Hey @georgewrmarshall : the goal of this PR is to extract this component out of confirm-transaction-base. It is bit beyond the scope of this RP to make it more re-usable.
ui/pages/confirm-deploy-contract/confirm-deploy-contract.component.js
Outdated
Show resolved
Hide resolved
Builds ready [f7d716f]
Page Load Metrics (1587 ± 40 ms)
Bundle size diffs
|
|
|
||
| ## Props | ||
|
|
||
| <ArgsTable of={ConfirmData} /> |
There was a problem hiding this comment.
I think we can just remove the README.mdx files altogether. They're only useful if we really want to dig deep on docs for a component otherwise it will generate a basic one. I'll update the storybook docs to define that apologies.
There was a problem hiding this comment.
Hey I tried to do that, but locally that gives issue @georgewrmarshall
|
|
||
| ## Props | ||
|
|
||
| <ArgsTable of={ConfirmHexData} /> |
There was a problem hiding this comment.
I think we can just remove this file altogether and remove the reference from the storybook file
Fixes: #17419
Extract out components to render transaction data and hexData from confirm-transaction-base component.