Skip to content
This repository has been archived by the owner on Dec 14, 2021. It is now read-only.

String updates to prepare for localization #617

Merged
merged 17 commits into from
May 1, 2019
Merged

Conversation

devinreams
Copy link
Contributor

@devinreams devinreams commented Apr 23, 2019

Fixes #579 from @Delphine and @Pike

Testing and Review Notes

This has just been text edits, not actually tested in the app yet and no string replacement for the app name.

To Do

  • double check the original issue to confirm it is fully satisfied
  • hook up the application name string replacement (%1$s for "Firefox Lockbox" via app_name)
    • app_logo
    • welcome_instructions
    • autofill_summary
    • fingerprint_dialog_title
    • unlock_fallback_title
    • enable_fingerprint_dialog_subtitle
    • no_device_security_message
    • disconnect_button
    • disconnect_disclaimer
    • disconnect_disclaimer_title
    • disconnect_disclaimer_message
    • password_for
    • no_entries_description
    • onboarding_unlock_title
    • onboarding_unlock_description
    • onboarding_autofill_title
    • onboarding_autofill_description
    • onboarding_autofill_image_description
    • security_content_description
    • autofill_authenticate_cta
    • autofill_search_cta
    • autofill_error_toast
  • add testing notes and screenshots in PR description to help guide reviewers
    • actually test and confirm the screens still work as expected
  • request the "UX" team perform a design review (if/when applicable)
  • make sure CI builds are passing (e.g.: fix lint and other errors)

In other mobile products we tend to use a placeholder, just in case: '%1$S' - and we always have a note in the comments saying the placeholder will be replaced by the app name

OK, I have done the changes in the strings file but not the corresponding replacement throughout the app yet. (and I'm open for anyone to help out with this)

Mentions of Firefox Account and Firefox

Added note on the "Firefox Account" translation expectations in comments

Apostrophes: we use curly quotes

Thanks, I think we got them all updated here now.

Formatted as comments instead of strings

We left those strings in for the network retry feature, to reserve them for future use. I moved and noted this to help explain it clearly but let me know if a complete removal is preferred...

@devinreams devinreams requested review from Pike and Delphine April 23, 2019 14:56
@devinreams devinreams self-assigned this Apr 23, 2019
@ghost ghost added the in progress label Apr 23, 2019
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@devinreams
Copy link
Contributor Author

OK the strings and comments are where we'd like them.

Now I'm a bit stuck needing help understanding how to properly parameterize/pull the app_name into the strings that expect it both in Kotlin and in the XML fragments.

Copy link
Contributor

@Delphine Delphine left a comment

Choose a reason for hiding this comment

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

This now LGTM from a string review perspective. Thanks Devin! :)

Copy link
Contributor

@Pike Pike left a comment

Choose a reason for hiding this comment

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

This looks ok from the string files, but the test crashes ... are they triggered by the changes in this patch?

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@devinreams devinreams marked this pull request as ready for review May 1, 2019 15:08
@devinreams devinreams requested a review from a team as a code owner May 1, 2019 15:08
Copy link
Contributor

@eliserichards eliserichards left a comment

Choose a reason for hiding this comment

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

LGTM! 🥇

@devinreams
Copy link
Contributor Author

OK we have approvals from l10n and engineering and CI builds are passing with master branch itself back to passing.

I am testing on device right now from another UI/UX test (@sashei had done this herself too) then merge to master. Thanks, everyone! 🤘

@devinreams
Copy link
Contributor Author

devinreams commented May 1, 2019

Confirmed locally* that string replacement with app name is now working as expected on: Unlock dialog, No Entries description, Unlock app system prompt. 👍

* with a merge against master locally, just now kicked off again to make super sure everything goes green on bitrise 🍏

@devinreams devinreams merged commit c1ba85d into master May 1, 2019
@devinreams devinreams deleted the 579-strings-updates branch May 1, 2019 19:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

String updates for first L10n export (2nd review)
5 participants