Skip to content
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

do not show account mismatch alert on details #8678

Merged
merged 1 commit into from
May 28, 2020

Conversation

brad-decker
Copy link
Contributor

adds a new prop that is true by default that controls whether or not to render the account mismatch alert in the SenderToRecipient component.

@brad-decker brad-decker requested a review from a team as a code owner May 28, 2020 15:37
@brad-decker brad-decker force-pushed the remove-alert-from-details branch from d3008a1 to 1d9fd23 Compare May 28, 2020 16:04
@metamaskbot
Copy link
Collaborator

Builds ready [1d9fd23]
Page Load Metrics (893 ± 78 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaintNaNNaNNaNNaNNaN
domContentLoaded512112889116378
load513113189316378
domInteractive512112889016378

@@ -22,6 +22,7 @@ function SenderAddress ({
senderName,
onSenderClick,
senderAddress,
warnUserOnAccountMismatch = true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Conventionally we use defautProps to specify defaults. Better to have once place to set defaults than two, and defaultProps lets us assign them the same way between functional and class components.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same applies to the SenderToRecipient component, though I'm not sure we need the default in two places 🤔 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove the defaultProps on the child component, and move them to use the actual defaultProps mechanism. I thought I remembered that default variables like what I did here were what TypeScript liked... memory is fickle.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That does sound plausible. Certainly open to changing the convention if it makes the TypeScript migration easier.

@brad-decker brad-decker force-pushed the remove-alert-from-details branch from 1d9fd23 to 41e3c63 Compare May 28, 2020 17:59
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@metamaskbot
Copy link
Collaborator

Builds ready [41e3c63]
Page Load Metrics (726 ± 69 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint32204654321
domContentLoaded420104772514369
load422104972614369
domInteractive420104772414369

@brad-decker brad-decker merged commit 00834f5 into develop May 28, 2020
@brad-decker brad-decker deleted the remove-alert-from-details branch May 28, 2020 19:20
Gudahtt added a commit that referenced this pull request Jun 1, 2020
* origin/develop: (689 commits)
  Implement asset page (#8696)
  fix crash on signature request (#8709)
  Fix accounts menu styling (#8707)
  Delete docs/porting_to_new_environment.md (#8704)
  Remove unused `getToErrorObject` parameters (#8705)
  hide connected-status on metamask ext (#8703)
  Stop adding permissions middleware to trusted connections (#8701)
  Use `send` state for send flow token (#8695)
  do not display extension id in connection modal (#8699)
  Fix tab content disappearing during scrolling on macOS Firefox (#8702)
  close details when button is pressed (#8694)
  Refactor token selectors (#8671)
  Update eth_accounts permission description (#8693)
  Extract selected token from token input (#8692)
  Fix propType for Home defaultHomeActiveTabName (#8683)
  Fix create account form styling (#8689)
  Remove unused `getSelectedTokenAssetImage` selector (#8691)
  Remove `getTxParams` (#8676)
  do not show account mismatch alert on details (#8678)
  Fix connect hardware styling (#8680)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants