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

Use different copy for self verification #3624

Merged
merged 3 commits into from
Jul 5, 2021
Merged

Conversation

dbkr
Copy link
Member

@dbkr dbkr commented Jul 2, 2021

The copy all refers to 'their' device for when you're verifying
someone else. Update it to refer to your own devices in the case
where you're verifying one of your own devices.

Pull Request Checklist

  • Changes has been tested on an Android device or Android emulator with API 21
  • UI change has been tested on both light and dark themes (Hopefully not necessary for a string change...)
  • Pull request is based on the develop branch
  • Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
  • Pull request includes screenshots or videos if containing UI changes
  • Pull request includes a sign off

Screenshot_2021-07-05-19-48-43-131_im vector app debug

Equivalent for iOS: element-hq/element-ios#4525

The copy all refers to 'their' device for when you're verifying
someone else. Update it to refer to your own devices in the case
where you're verifying one of your own devices.
@dbkr dbkr assigned dbkr and unassigned dbkr Jul 2, 2021
@dbkr dbkr marked this pull request as ready for review July 2, 2021 19:22
Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.
One remark: the "Can't scan" option is also a bit weird when self-verifying :/. We should maybe hide the subtitle in this case?

@dbkr
Copy link
Member Author

dbkr commented Jul 5, 2021

oh, good point

and tweak the other string more because it's not "each other" either
@dbkr
Copy link
Member Author

dbkr commented Jul 5, 2021

updated

var scanOtherCodeTitle: String
var compareEmojiSubtitle: String
if (state.isMe) {
scanCodeInstructions = host.stringProvider.getString(R.string.verification_scan_self_notice)
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I did not noticed it before, but there is no need to use host. here. It only useful if you want to use the host member from the Epoxy item DSL, for instance inside bottomSheetVerificationNoticeItem {} block.
It's not a big deal at all, it's OK to merge the PR like that, I can do the clean up later.

Copy link
Member

Choose a reason for hiding this comment

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

(Fixed in 07d6eaa)

Copy link
Member Author

Choose a reason for hiding this comment

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

ah - I see, I did wonder what that was all about, but figured it was best to be consistent anyhow

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Thanks for the update, LGTM

@bmarty bmarty merged commit a335c5e into develop Jul 5, 2021
@bmarty bmarty deleted the dbkr/scan_button_title branch July 5, 2021 19:25
bmarty added a commit that referenced this pull request Jul 5, 2021
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.

2 participants