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. |
ryanml
left a comment
There was a problem hiding this comment.
left one comment, else lgtm!
| width={image.width} | ||
| /> | ||
| )} | ||
| {image && !image.placeBelowDescription && imageComponent} |
There was a problem hiding this comment.
The leading check for image shouldn't be necessary given that it's already checked for when imageComponent is declared: https://github.com/MetaMask/metamask-extension/pull/10964/files#diff-8a1b237ca446aed86e9d755decb615e0f1b7b3034bf4920781bc494a81aff657R35
Builds ready [f1579b9]
Page Load Metrics (677 ± 47 ms)
|
|
Small thing I noticed. The other modals have 16px padding. Looks like this is 24px padding. I think it would be worth updating to 16px to give the content in the modal a little more room
|
|
Addressed design feedback: |
|
This looks good to me! |
| </div> | ||
| <div className="whats-new-popup__notification-date">{date}</div> | ||
| </div> | ||
| {imageComponent && placeImageBelowDescription ? imageComponent : null} |
There was a problem hiding this comment.
Could this be refactored to {placeImageBelowDescription && imageComponent}?
There was a problem hiding this comment.
yes, thanks, refactored and pushed 7c8fe41
| width={image.width} | ||
| /> | ||
| )} | ||
| {!placeImageBelowDescription && imageComponent ? imageComponent : null} |
There was a problem hiding this comment.
Could this be refactored to {!placeImageBelowDescription && imageComponent}?
There was a problem hiding this comment.
yes, thanks, refactored and pushed 7c8fe41
Builds ready [7c8fe41]
Page Load Metrics (646 ± 57 ms)
|
Makes some tweaks to the Whats New popup UI. Based on feedback from Thomas and a convo with Rachel:
Peek.2021-04-30.17-11.mp4