-
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-14019] Toggle Vault Filters #11929
Conversation
- spacing will be managed by the parent component
- I noticed delays when trying to use stored state as the source of truth
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #11929 +/- ##
==========================================
+ Coverage 33.46% 33.52% +0.06%
==========================================
Files 2858 2859 +1
Lines 89396 89434 +38
Branches 17018 17022 +4
==========================================
+ Hits 29919 29986 +67
+ Misses 57120 57087 -33
- Partials 2357 2361 +4 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
No New Or Fixed Issues Found |
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.
Could we use a two-way binding instead of a separate output?
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.
Nice! 69713d0
I also refactored a little bit to make sure there wasn't duplicate class/state updates in this same commit.
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.
Looking great! Just a few suggestions and a possible missing border issue.
apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts
Outdated
Show resolved
Hide resolved
apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts
Outdated
Show resolved
Hide resolved
apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html
Outdated
Show resolved
Hide resolved
apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html
Outdated
Show resolved
Hide resolved
- also adjust consuming changes
…t/pm-14019/hide-vault-filters
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.
Looking good! I just had two other suggestions after taking another look. Let me know what you think!
@@ -73,6 +82,9 @@ export class VaultPopupListFiltersService { | |||
shareReplay({ refCount: true, bufferSize: 1 }), | |||
); | |||
|
|||
/** Stored state for the visibility of the filters. */ | |||
filterVisibilityState = this.stateProvider.getGlobal(FILTER_VISIBILITY_KEY); |
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.
🎨 I think it would be preferable to avoid exposing the "raw" state on the service and instead have the service provide an public observable stream and public method to toggle it the state's value. What do you think?
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.
I debated the same, thanks for pushing me in the right direction! 3a9eb5f
apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts
Show resolved
Hide resolved
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.
✅ Verified visuals for expanded/collapsed icon button look as expected
✅ Verified a badge is added when user applies a filter and collapses the section
✅ Verified screen reader behavior works in Chrome with VO
6869f70
to
3a9eb5f
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.
Looks great, nice work! I do see that we have some failing tests now though
apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts
Outdated
Show resolved
Hide resolved
…t/pm-14019/hide-vault-filters
…t/pm-14019/hide-vault-filters
@willmartian @Jingo88 @cd-bitwarden @gbubemismith Shane is out, I apologize for the ping. The only update since his approval is using the Tailwind colors now that |
🎟️ Tracking
PM-14019
📔 Objective
Add the ability to toggle the visibility of the vault filters
Notes
notification-100
¬ification-600
are only available in theps/extension-refresh
branch.ps/extension-refresh
branch and will then move this to testing.DisclosureComponent
so I could track the state from the consuming component.open
attribute an observable. We can discuss if an other approach would be better.📸 Screenshots
filter-badges.mov
filter-persist-state.mov
filter-screen-reader.mov
🦮 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