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

i18n - Removes t from props via metamask-connect; put t on context #3788

Merged
merged 5 commits into from
Mar 29, 2018
Merged

i18n - Removes t from props via metamask-connect; put t on context #3788

merged 5 commits into from
Mar 29, 2018

Conversation

danjm
Copy link
Contributor

@danjm danjm commented Mar 29, 2018

Fixes #3787

See issue #3787 for details about this. This seems to be working in all places. We should give it another thorough round of QA though.

@danjm danjm requested a review from alextsg March 29, 2018 15:03
@kumavis
Copy link
Member

kumavis commented Mar 29, 2018

e2e error seems really consistent, retried it a few times

@kumavis
Copy link
Member

kumavis commented Mar 29, 2018

confirmed the QR code view on old-ui does not render (but no error?)
Edit: actually the test does not look for the actual QR code, it just checks that the option is in the menu

@danjm
Copy link
Contributor Author

danjm commented Mar 29, 2018

@danjm danjm added the DO-NOT-MERGE Pull requests that should not be merged label Mar 29, 2018
@kumavis
Copy link
Member

kumavis commented Mar 29, 2018

As an experiment i merged this with #3798
which produces artifacts for test failures https://circleci.com/gh/MetaMask/metamask-extension/9957#artifacts/containers/0

here's the first one:
test-failure-screenshot

@kumavis kumavis changed the title Removes t from props via metamask-connect; put t on context i18n - Removes t from props via metamask-connect; put t on context Mar 29, 2018
@danjm
Copy link
Contributor Author

danjm commented Mar 29, 2018

Need to update context after changing language according to the advice here:

I did further research (e.g. facebook/react#2517) and I think what we currently have is good. We just need to make sure that we don't use shouldComponentUpdate until we switch to react 16.3

shouldComponentUpdate is not used anywhere in our code base yet. Its purpose is to optimize performance, and so is used in often in libraries that are optimizing for that or apps where the combo of component tree size and state update frequency make performance an issue.

I don't think we'll need to worry about it in the near future, and react 16.3 will be production ready in the near future. Meanwhile, local testing now shows consistent and successful updating behaviour. (I think my build was messed up before.)

@kumavis
Copy link
Member

kumavis commented Mar 29, 2018

somehow CI did not get triggered 🤔

@kumavis kumavis merged commit 9d4be18 into MetaMask:master Mar 29, 2018
@kumavis
Copy link
Member

kumavis commented Mar 29, 2018

👏 👏 👏 👏

@tmashuang tmashuang removed the DO-NOT-MERGE Pull requests that should not be merged label Mar 30, 2018
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