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.

Sign up for GitHub

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

Jump to bottom

Gekidou about screen #5591

Merged
merged 15 commits into from
Aug 5, 2021
Merged

Gekidou about screen #5591

merged 15 commits into from
Aug 5, 2021

Conversation

avinashlng1080
Copy link
Contributor

Summary

Ported the About Screen and I did a little bit of refactoring

@mm-cloud-bot
Copy link

@avinashlng1080: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

I understand the commands that are listed here

@avinashlng1080 avinashlng1080 requested review from enahum and migbot August 4, 2021 09:24
@avinashlng1080 avinashlng1080 added 2: Dev Review Requires review by a core commiter and removed do-not-merge/release-note-label-needed labels Aug 4, 2021
@mm-cloud-bot
Copy link

@avinashlng1080: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

I understand the commands that are listed here

@avinashlng1080
Copy link
Contributor Author

/release-note-none

@avinashlng1080 avinashlng1080 marked this pull request as ready for review August 4, 2021 09:40
enahum
enahum requested changes Aug 4, 2021
View reviewed changes
Copy link
Contributor

@enahum enahum left a comment

Choose a reason for hiding this comment

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

Do not forget to run npm run i18n-extract and check for empty strings under assets/base/i18n/en.json file, add the right values to the en.json if they are missing.

app/screens/about/index.tsx Outdated Show resolved Hide resolved
app/screens/about/index.tsx Outdated Show resolved Hide resolved
app/screens/about/learn_more.tsx Outdated Show resolved Hide resolved
app/screens/about/subtitle.tsx Outdated Show resolved Hide resolved
app/screens/about/learn_more.tsx Outdated Show resolved Hide resolved
app/screens/about/subtitle.tsx Outdated Show resolved Hide resolved
app/screens/about/title.tsx Outdated Show resolved Hide resolved
app/screens/about/title.tsx Outdated Show resolved Hide resolved
app/screens/about/title.tsx Outdated Show resolved Hide resolved
avinashlng1080 and others added 3 commits August 4, 2021 20:28
@avinashlng1080
Copy link
Contributor Author

Do not forget to run npm run i18n-extract and check for empty strings under assets/base/i18n/en.json file, add the right values to the en.json if they are missing.

@enahum Although I did this, I'm still seeing the MISSING_TRANSLATION error on the simulator. All the ids of the FormattedText component have been wrapped with t() as well. Is there an additional step? I did reset the simulator itself + the nodejs server.

@enahum
Copy link
Contributor

enahum commented Aug 4, 2021

@enahum Although I did this, I'm still seeing the MISSING_TRANSLATION error on the simulator. All the ids of the FormattedText component have been wrapped with t() as well. Is there an additional step? I did reset the simulator itself + the nodejs server.

@avinashlng1080 run node scipts/generate-assets.js and the error will go away (normally this command is executed with by npm install)

@avinashlng1080
Copy link
Contributor Author

@enahum Although I did this, I'm still seeing the MISSING_TRANSLATION error on the simulator. All the ids of the FormattedText component have been wrapped with t() as well. Is there an additional step? I did reset the simulator itself + the nodejs server.

@avinashlng1080 run node scipts/generate-assets.js and the error will go away (normally this command is executed with by npm install)

Thanks. It works !

enahum
enahum reviewed Aug 4, 2021
View reviewed changes
app/screens/about/server_version.tsx Outdated
};

if (buildNumber === version) {
id = 'mobile.about.serverVersionNoBuild';
Copy link
Contributor

Choose a reason for hiding this comment

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

missed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

@avinashlng1080 avinashlng1080 requested a review from enahum August 4, 2021 18:21
@migbot migbot added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Aug 5, 2021
@migbot migbot merged commit 0ef1dd4 into gekidou Aug 5, 2021
@migbot migbot deleted the gekidou_about_screen branch August 5, 2021 04:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enahum enahum enahum approved these changes

@migbot migbot migbot approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request release-note-none
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@avinashlng1080 @mm-cloud-bot @enahum @migbot