[PM-15999] - use new generator components in desktop app #12639
Conversation
|
No New Or Fixed Issues Found |
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #12639 +/- ##
==========================================
- Coverage 33.76% 33.72% -0.05%
==========================================
Files 2912 2919 +7
Lines 90791 91026 +235
Branches 17177 17207 +30
==========================================
+ Hits 30655 30696 +41
- Misses 57743 57930 +187
- Partials 2393 2400 +7 ☔ View full report in Codecov by Sentry. |
audreyality
requested review from
audreyality
and removed request for
djsmith85
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@audreyality Thanks for your feedback. Should be gtg now. ;) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Thanks for pointing that out. I've updated the component to instead use the |
| ); | ||
|
|
||
| if (isGeneratorSwapEnabled) { | ||
| this.dialogService.open(CredentialGeneratorComponent, { |
There was a problem hiding this comment.
🎨 (non-blocking): Our dialogue components usually have a static open method that takes the dialog service and params as an argument. I think we can do something similar to keep things consistent.
There was a problem hiding this comment.
@gbubemismith Gotcha. I was considering this but wasn't sure whether or not to keep the component as-is to allow for re-use in non dialog contexts. I've updated it to follow the existing dialog component patterns. :)
| } | ||
|
|
||
| // TODO: Remove this when new generator feature is enabled |
There was a problem hiding this comment.
❓ is there a ticket tracking this?
There was a problem hiding this comment.
@gbubemismith Totally agree, I don't like leaving TODOs without an accompanying ticket. I've found and referenced the ticket number.
| @@ -21,10 +21,19 @@ export type CredentialGeneratorParams = { | |||
| type: "password" | "username"; | |||
| }; | |||
|
|
|||
| export const openCredentialGeneratorDialog = ( | |||
There was a problem hiding this comment.
we need this in the component
static open(dialogService: DialogService, params: CredentialGeneratorParams) {}and then you reference the static method in the vault component like so
CredentialGeneratorDialogComponent.open()There was a problem hiding this comment.
@gbubemismith Gotcha! I was looking at a slightly different implementation. Should be gtg now.
jaasen-livefront
requested review from
audreyality
and removed request for
nick-livefront
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-15999
📔 Objective
This PR replaces the legacy credential generators with the new ones in the desktop app.
📸 Screenshots
Screen.Recording.2024-12-31.at.9.41.29.AM.mov
⏰ Reminders before review
🦮 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