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

Add confirmation dialog when switching target lang with unsaved changes #51

Merged
merged 1 commit into from
Jan 21, 2019

Conversation

samwilson
Copy link
Member

When a user switches the target language but has unsaved changes in
the translation form, show a dialog box asking if they're sure they
want to lose their work.

Bug: https://phabricator.wikimedia.org/T207199

@samwilson samwilson added the WIP Work in progress label Jan 11, 2019
@coveralls
Copy link

Pull Request Test Coverage Report for Build 536

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 77.707%

Totals Coverage Status
Change from base Build 492: 0.0%
Covered Lines: 488
Relevant Lines: 628

💛 - Coveralls

@samwilson samwilson added ready for review Please review this code and removed WIP Work in progress labels Jan 15, 2019
When a user switches the target language but has unsaved changes in
the translation form, show a dialog box asking if they're sure they
want to lose their work.

Bug: https://phabricator.wikimedia.org/T207199
@@ -29,6 +29,8 @@
"default-language": "Default language",
"source-lang-not-found": "Untranslated. Default: $1",
"source-to-target": "to",
"confirmation-to-switch-target-lang": "You are trying to change the translation language but you have unsaved translations. Changing the language will result in loss of any added translations. Please upload the translations to Commons or download the translated file before proceeding. Do you wish to continue?",
"confirm-change-target-lang": "Change language",
Copy link
Member

@MusikAnimal MusikAnimal Jan 18, 2019

Choose a reason for hiding this comment

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

Why do we duplicate the translations in /i18n/ here in /public/assets/i18n?

Copy link
Member

Choose a reason for hiding this comment

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

Could you use a symlink instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, becuase one of the copied dirs is node_modules/jquery.uls/i18n which doesn't exist in the repo.

Copy link
Member

@MusikAnimal MusikAnimal left a comment

Choose a reason for hiding this comment

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

Works on my local!

I had a question about duplicating the /i18n/ translations to /public/assets/i18n/, but that's unrelated to this PR.

@samwilson samwilson merged commit afea623 into master Jan 21, 2019
@samwilson samwilson deleted the confirm-switch-T207199 branch January 21, 2019 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review Please review this code
Development

Successfully merging this pull request may close these issues.

3 participants