Review spending cap screen #15919
Review spending cap screen #15919
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. |
filipsekulic
force-pushed
the
review-spending-cap-screen
branch
from
ebfdfa5 to
087a947
Compare
Builds ready [e49ab08]
Page Load Metrics (1437 ± 89 ms)
highlights:storybook
|
filipsekulic
force-pushed
the
review-spending-cap-screen
branch
from
9fd172f to
df35fa8
Compare
Builds ready [d90bade]
Page Load Metrics (2179 ± 77 ms)
highlights:storybook
|
|
Verified by QA |
Builds ready [d588c5e]
Page Load Metrics (2540 ± 110 ms)
highlights:storybook
|
filipsekulic
force-pushed
the
review-spending-cap-screen
branch
from
3582ed4 to
6c593ba
Compare
Builds ready [6c593ba]
Page Load Metrics (2481 ± 209 ms)
highlights:storybook
|
There was a problem hiding this comment.
Really nice work!! Great use of UI components and clean CSS. I'm just dealing with a minor bug and and issue with the full screen view
Minor: Copy to clipboard state seems to be shared between the components here
copytoclipboard.mov
When I approve tokens my MetaMask opens in full screen and when I use the open in explorer it does open but it doesn't change window. Not sure how one might fix this is there a way to change windows?
explorer.not.swapping.on.full.screen.mov
Maybe a way we can set the href instead of opening it with javascript and using the attr target="_blank"? Not sure how to do this just spit balling...
ui/components/app/modals/contract-details-modal/contract-details-modal.js
Outdated
Show resolved
Hide resolved
| export default function ContractDetailsModal({ | ||
| onClose, | ||
| tokenName, | ||
| tokenAddress, | ||
| toAddress, | ||
| chainId, | ||
| rpcPrefs, | ||
| origin, | ||
| siteImage, | ||
| }) { |
There was a problem hiding this comment.
Can we add these props to the args in storybook so it doesn't break? Seems to be working nicely on develop See github pages storybook
There was a problem hiding this comment.
I really need to add storybook checks to the CI 😅 I have a ticket will try get to it this week 🙏
| </Box> | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Any chance we could get a story for this component? I know it's going to take a lot of mock data but if it's not too much overhead I think it could add a lot of value!
filipsekulic
force-pushed
the
review-spending-cap-screen
branch
from
d276b10 to
6ce3438
Compare
|
Hey @georgewrmarshall! Good catch! 🙌 Screen.Recording.2022-09-29.at.18.02.50.mov |
Builds ready [6ce3438]
Page Load Metrics (2359 ± 128 ms)
highlights:storybook
|
Builds ready [984fdfb]
Page Load Metrics (2225 ± 71 ms)
highlights:storybook
|
ui/pages/token-allowance/index.scss
Outdated
| &__card, | ||
| &__card--no-border { | ||
| display: flex; | ||
| flex-flow: column; | ||
| border-bottom: 1px solid var(--color-border-default); | ||
| position: relative; | ||
| padding-inline-start: 24px; | ||
| padding-inline-end: 24px; | ||
| } |
There was a problem hiding this comment.
Some of these styles can be achieved with props on the box component. Please remove any styles that can be expressed as props to avoid duplicating styles in our styelsheet. @georgewrmarshall we should probably create an issue for adding padding/margin inline-start and linline-end support to box.
There was a problem hiding this comment.
Oh nice spotting. Roger that! I will create an issue *edit #16077
There was a problem hiding this comment.
Fixed here: db22c9f
| const renderFullDetails = () => { | ||
| return ( | ||
| <Box | ||
| display={DISPLAY.FLEX} | ||
| flexDirection={FLEX_DIRECTION.COLUMN} | ||
| alignItems={ALIGN_ITEMS.CENTER} | ||
| className="token-allowance-container__full-tx-content" |
There was a problem hiding this comment.
We shouldn't create functions that render more of the tree inside of functional components. Either render it inline in the component or extract it to a new component. When you reach for this it is a good sign that you need to break down your component.
There was a problem hiding this comment.
Fixed here: db22c9f
| const renderApproveContentCard = ({ | ||
| showHeader = true, | ||
| symbol, | ||
| title, | ||
| showEdit, | ||
| showAdvanceGasFeeOptions = false, | ||
| onEditClick, | ||
| content, | ||
| footer, | ||
| noBorder, | ||
| }) => { | ||
| return ( | ||
| <Box | ||
| className={classnames({ | ||
| 'token-allowance-container__card': !noBorder, | ||
| 'token-allowance-container__card--no-border': noBorder, | ||
| })} |
There was a problem hiding this comment.
These functions that render things are all their own react components that you are creating inside of a react component. We do not want this pattern to exist in the code base. Extract these things to their own files / components even if they will not be reused. In this case it is used at least twice
There was a problem hiding this comment.
Fixed here: db22c9f
filipsekulic
force-pushed
the
review-spending-cap-screen
branch
from
984fdfb to
9a29ba4
Compare
filipsekulic
force-pushed
the
review-spending-cap-screen
branch
from
9a29ba4 to
db22c9f
Compare
I'll then add the missing storybook in the following PR where this |
Builds ready [7382be0]
Page Load Metrics (2210 ± 72 ms)
highlights:storybook
|
There was a problem hiding this comment.
Agree with @brad-decker now that the ApproveContentCard is a separate component it would be nice to get a story for it. But I see you have addressed that comment so not a blocker. Nice work 👍
| showHeader: PropTypes.bool, | ||
| symbol: PropTypes.node, | ||
| title: PropTypes.string, | ||
| showEdit: PropTypes.bool, | ||
| showAdvanceGasFeeOptions: PropTypes.bool, | ||
| onEditClick: PropTypes.func, | ||
| footer: PropTypes.node, | ||
| noBorder: PropTypes.bool, | ||
| supportsEIP1559V2: PropTypes.bool, | ||
| renderTransactionDetailsContent: PropTypes.bool, | ||
| renderDataContent: PropTypes.bool, | ||
| isMultiLayerFeeNetwork: PropTypes.bool, | ||
| ethTransactionTotal: PropTypes.string, | ||
| nativeCurrency: PropTypes.string, | ||
| fullTxData: PropTypes.object, | ||
| hexTransactionTotal: PropTypes.string, | ||
| fiatTransactionTotal: PropTypes.string, | ||
| currentCurrency: PropTypes.string, | ||
| isSetApproveForAll: PropTypes.bool, | ||
| isApprovalOrRejection: PropTypes.bool, | ||
| data: PropTypes.string, |
There was a problem hiding this comment.
PropType descriptions would be nice here
There was a problem hiding this comment.
I'm gonna squash and merge, @filipsekulic please add those prop type descriptions in the same pr you mentioned above.
Explanation
Created a new screen - 'Review Spending Cap' screen.
More Information
Screenshots/Screencaps
Before
Before.mov
After
After.mov
Manual Testing Steps
BEFORE TESTING - in your
.metamaskrcfile, setTOKEN_ALLOWANCE_IMPROVEMENTS=1and then start the appSend Tokensclick onCREATE TOKENAPPROVE TOKENSorAPPROVE TOKENS WITHOUT GASin order to see the newly created screenTo see how the screen looks like when the enhanced gas fee is enabled, go to
Settings (Experimental tab)and enable it (Enable enhanced gas fee UItoggle).