-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[PM-16804] Add supporting Vault component presentational updates for blocked domains #12720
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
โ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #12720 +/- ##
==========================================
- Coverage 34.14% 34.12% -0.02%
==========================================
Files 2937 2937
Lines 90337 90483 +146
Branches 16961 17000 +39
==========================================
+ Hits 30846 30879 +33
- Misses 57036 57146 +110
- Partials 2455 2458 +3 โ View full report in Codecov by Sentry. |
Fixed Issues
|
8356aae
to
580bf9f
Compare
66dba77
to
b7e5851
Compare
There was a problem hiding this 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! Thanks encapsulating the logic in a new banner component.
There are a few issues around reactivity and the Firefox sidebar that we'll need to address, but otherwise I just left a few other suggestions/nits.
...popup/components/vault-v2/autofill-vault-list-items/autofill-vault-list-items.component.html
Show resolved
Hide resolved
* Flag indicating that the current tab location is blocked | ||
*/ | ||
@Input({ transform: booleanAttribute }) | ||
currentURIIsBlocked: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
๐จ Instead of an input property, we can use the already injected VaultPopupAutofillService
and reference the currentTabIsOnBlocklist$
observable.
We should also make use of the observable and an async
pipe in the template as the current tab could change while the extension is open. (e.g. the Firefox sidebar has a "refresh current tab" button that will cause currentAutofillTab$
to re-emit)
@@ -22,6 +22,11 @@ | |||
</bit-no-items> | |||
</div> | |||
|
|||
<blocked-injection-banner |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
๐ Perfect!
/** | ||
* Flag indicating that the banner should be shown | ||
*/ | ||
protected showScriptInjectionIsBlockedBanner = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currentAutofillTab$
observable these all depend on internally could re-emit within the lifetime of Vault page / this banner component.
The Firefox sidebar has a "refresh current tab" button that will force currentAutofillTab$
to re-emit and if we're now on a blocked tab (or vice versa) we'd want to have the UI be reactive.
protected showScriptInjectionIsBlockedBanner = false; | |
protected showScriptInjectionIsBlockedBanner$ = this.vaultPopupAutofillService.currentTabBlockedBannerIsDismissed$; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -74,6 +72,10 @@ export class AutofillVaultListItemsComponent implements OnInit { | |||
} | |||
|
|||
async ngOnInit() { | |||
this.currentURIIsBlocked = await firstValueFrom( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I think we just have one last property to make reactive for current tab changes.
/** | ||
* Flag indicating that the current tab location is blocked | ||
*/ | ||
currentURIIsBlocked: boolean = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yeah, good call; sorted in fc3823f
๐๏ธ Tracking
PM-16804
๐ Objective
Split from #11826 - see details in original PR
๐ฆฎ Reviewer guidelines
:+1:
) or similar for great changes:memo:
) or โน๏ธ (:information_source:
) for notes or general info:question:
) for questions:thinking:
) or ๐ญ (:thought_balloon:
) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:
) for suggestions / improvements:x:
) or:warning:
) for more significant problems or concerns needing attention:seedling:
) or โป๏ธ (:recycle:
) for future improvements or indications of technical debt:pick:
) for minor or nitpick changes