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

[Credentials Import] Add "Don't Show Again" button to import prompt #676

Merged
merged 13 commits into from
Oct 14, 2024

Conversation

dbajpeyi
Copy link
Collaborator

@dbajpeyi dbajpeyi commented Oct 8, 2024

Reviewer: @GioSensation
Asana: https://app.asana.com/0/1205996472158114/1208381175784446/f

Description

As the Win and Mac plan to release this prompt to existing users and updating some heuristics to be more persistent, we are adding new UI and API call for dismissing credentials import dialog. Introduces a new API call.

Screenshot 2024-10-09 at 16 52 27

Steps to test

  1. Build autofill with the tip of the branch -graeme/enable-import-prompt-for-all-users on MacOS browser,
  2. Reset autofill data from debug menu and also set the activation date to today,
  3. Click on one of the login inputs on https://fill.dev/form/login-simple, credentials import prompt should appear,
  4. Click on "Don't show again" - password import prompt will disappear,
  5. On clicking the inputs again, the prompt shouldn't re-appear (and also after refresh).

@dbajpeyi dbajpeyi marked this pull request as ready for review October 9, 2024 14:51
@dbajpeyi dbajpeyi force-pushed the dbajpeyi/feature/dismiss-password-import-flow branch from 4c04430 to cc23669 Compare October 9, 2024 15:42
@dbajpeyi dbajpeyi changed the title Dbajpeyi/feature/dismiss password import flow [Credentials Import] Add "Don't Show Again" button to import prompt Oct 10, 2024
@dbajpeyi dbajpeyi force-pushed the dbajpeyi/feature/dismiss-password-import-flow branch from 177bf7a to 45f6716 Compare October 10, 2024 10:28
@@ -215,7 +215,7 @@
color: #FFFFFF;
}

.tooltip__button--manage {
.tooltip__button--secondary {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just renaming it to sound more "reusable", just because we have the same scenario (manage password + don't show again) kind of like a secondary button.

Copy link
Member

Choose a reason for hiding this comment

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

Nice.

Copy link
Member

@GioSensation GioSensation left a comment

Choose a reason for hiding this comment

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

This is looking good. Nice little change. Just a tiny comment and the native-side bug I mentioned on MM.

@@ -215,7 +215,7 @@
color: #FFFFFF;
}

.tooltip__button--manage {
.tooltip__button--secondary {
Copy link
Member

Choose a reason for hiding this comment

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

Nice.

Comment on lines 63 to 64
this.device.deviceApi.notify(new CloseAutofillParentCall(null))
this.device.deviceApi.notify(new CredentialsImportFlowPermanentlyDismissedCall(null))
Copy link
Member

Choose a reason for hiding this comment

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

Just a thought. Here we call close, then the other call (same as we do above in line 58). Is there a race condition where the first call's side effect is so quick that the second call never happens because then the webview gets closed and the js context is destroyed? Should we flip their order?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good point! I think it's safer to do flipped order. Changed it in 79c1941

@dbajpeyi dbajpeyi merged commit ac93dec into main Oct 14, 2024
1 check passed
@dbajpeyi dbajpeyi deleted the dbajpeyi/feature/dismiss-password-import-flow branch October 14, 2024 15:48
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