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

Resolves brave/brave-browser#15251 #8558

Merged

Conversation

rahulsnkr
Copy link
Contributor

Resolves brave/brave-browser#15251

Fixes typos in the remove-device prompt text when a device is removed from the sync chain.

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

@stephendonner
Copy link
Contributor

stephendonner commented Apr 18, 2021

This looks good to me @rahulsnkr -- thanks for picking this up!

I don't know if these strings exist anywhere else; do you know/mind giving a review, @AlexeyBarabash ?

@AlexeyBarabash
Copy link
Contributor

Thanks @rahulsnkr !

English is not my native language, so I can't properly review it, I will add @brave/design team to check that.

Copy link
Collaborator

@AlanBreck AlanBreck left a comment

Choose a reason for hiding this comment

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

Not sure if @brave/design ultimately wants to change from delete to remove, but the English usage side LGTM.

@stephendonner
Copy link
Contributor

Not sure if @brave/design ultimately wants to change from delete to remove, but the English usage side LGTM.

Yup, I cleared the text change with @karenkliu before filing (so as to match the Remove column heading). 👍

Copy link
Contributor

@AlexeyBarabash AlexeyBarabash left a comment

Choose a reason for hiding this comment

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

Please revert app/resources/brave_generated_resources_en-GB.xtb

Copy link

@jsecretan jsecretan left a comment

Choose a reason for hiding this comment

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

lgtm

@rahulsnkr rahulsnkr force-pushed the fix-delete-device-from-sync-chain-msg branch from 4065544 to e9f0ba0 Compare April 20, 2021 14:26
Copy link
Contributor

@AlexeyBarabash AlexeyBarabash left a comment

Choose a reason for hiding this comment

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

++

@AlexeyBarabash
Copy link
Contributor

@rahulsnkr could you please merge it?

@rahulsnkr rahulsnkr requested a review from jsecretan April 21, 2021 13:25
@rahulsnkr
Copy link
Contributor Author

Apologies @jsecretan, accidentally re-requested a review.

@AlexeyBarabash I can't merge yet since @karenkliu and @jamesmudgett are yet to approve.

@AlexeyBarabash
Copy link
Contributor

Once @stephendonner agreed the changes with @karenkliu then I remove @karenkliu and @jamesmudgett from reviewers.

@rahulsnkr
Copy link
Contributor Author

@jsecretan could you please approve again?

@rahulsnkr
Copy link
Contributor Author

I still don't see the option to merge since I don't have write access to the repo @AlexeyBarabash

@AlexeyBarabash
Copy link
Contributor

oh, sorry, @rahulsnkr, I didn't know that. Then I will merge it. Thanks a lot for the contribution.

@AlexeyBarabash AlexeyBarabash merged commit 18f3f06 into brave:master Apr 23, 2021
@rahulsnkr rahulsnkr deleted the fix-delete-device-from-sync-chain-msg branch August 13, 2021 21:08
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.

Minor typos in remove-device dialog prompt text
6 participants