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

[Wallet] Don't log all props, which includes i18n #1445

Merged
merged 3 commits into from
Oct 23, 2019

Conversation

cmcewen
Copy link
Contributor

@cmcewen cmcewen commented Oct 23, 2019

Description

Since this component is connected to i18n, by doing ...this.props we ended up logging all the translations.

Tested

Ran locally

Other changes

Also whitelist tti property

Backwards compatibility

No effect

@cmcewen cmcewen requested a review from jmrossy as a code owner October 23, 2019 18:21
@cmcewen cmcewen changed the title Don't log all props, which includes i18n [Wallet] Don't log all props, which includes i18n Oct 23, 2019
Copy link
Contributor

@annakaz annakaz left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning up these logs

@cmcewen cmcewen added the automerge Have PR merge automatically when checks pass label Oct 23, 2019
@codecov
Copy link

codecov bot commented Oct 23, 2019

Codecov Report

Merging #1445 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1445      +/-   ##
==========================================
- Coverage   65.87%   65.86%   -0.01%     
==========================================
  Files         271      271              
  Lines        8061     8062       +1     
  Branches      490      490              
==========================================
  Hits         5310     5310              
- Misses       2639     2640       +1     
  Partials      112      112
Flag Coverage Δ
#mobile 65.86% <0%> (-0.01%) ⬇️
Impacted Files Coverage Δ
packages/mobile/src/analytics/constants.ts 100% <ø> (ø) ⬆️
...ckages/mobile/src/exchange/ExchangeTradeScreen.tsx 82.52% <0%> (-0.81%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be9a7f0...5d4d7c1. Read the comment docs.

@celo-ci-bot-user celo-ci-bot-user merged commit 87c5556 into master Oct 23, 2019
@celo-ci-bot-user celo-ci-bot-user deleted the cmcewen/exchange-log branch October 23, 2019 21:31
@jeanregisser
Copy link
Contributor

Nice! I had this on my list cause it was slowing down the app when opening that screen! Glad you fixed it 👍

aaronmgdr added a commit that referenced this pull request Oct 24, 2019
* master:
  [Wallet] Wallet can switch between hosted and local node (#1419)
  [Wallet] Prevent error from Avatar when name is missing (#1454)
  [Wallet] Show splash screen until JS is ready on iOS (#1453)
  Use new segment api keys used by both iOS and Android (#1452)
  [Wallet] Don't log all props, which includes i18n (#1445)
  [Helm] Updated the helm package to deploy the upgraded blockscout version (#1129)
  Tiny copy change (#1429)
  [contractkit] SortedOraclesWrapper + tests (#1405)
  [wallet] Refactor leftover thunk to sagas (#1388)
  [Wallet] Fix repeated QR code scanning and related navigation issues (#1439)
  [Wallet] Show the currency values with correct rounding. (#1435)
  [Wallet] Fix firebase initialization error on iOS after reinstalling the app (#1423)
  [Wallet] Use exit on iOS since we can't restart like Android (#1424)
  [Wallet] Update local currency styles and layout (#1325)
  Reset pincode cache if unlock fails (#1430)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Have PR merge automatically when checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants