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

[PM-1504] Migrate Dialogs to DialogService #5013

Merged
merged 28 commits into from
May 2, 2023
Merged

[PM-1504] Migrate Dialogs to DialogService #5013

merged 28 commits into from
May 2, 2023

Conversation

Hinton
Copy link
Member

@Hinton Hinton commented Mar 16, 2023

Type of change

- [ ] Bug fix
- [ ] New feature development
- [x] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

We need to migrate all the dialogs to the new simple dialog system on web.

This PR introduces a generic DialogService which can be used by all the clients. This allows us to decouple dialogs from the PlatformUtilsHelper.

The DialogService provides a new method, openSimpleDialog which is the new interface for that type of dialogs.

This gives us 3 different implementations:

  • Web: DialogService modern dialogs
  • Browser: SweetAlert
  • Desktop: Native electron based

Review notes

  • Check that each dialog that previously had await still has await.
  • Verify title and content hasn't accidentally flipped places. Confusingly the platformUtilsService had content first and title second.

Screenshots

Old:
image

New:
image

Before you submit

  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team
  • Ensure that all UI additions follow WCAG AA requirements

@github-actions github-actions bot added the needs-qa Marks a PR as requiring QA approval label Mar 16, 2023
title: this.i18nService.t("desktopSyncVerificationTitle"),
confirmText: this.i18nService.t("ok"),
type: "warning",
this.messagingService.send("showNativeMessagingFinterprintDialog", {
Copy link
Member Author

Choose a reason for hiding this comment

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

Not a huge fan of this approach, but since the new dialogs doesn't support html, this seemed like the way to get around it for now. I envision us writing a popup service companion to this background service in the future that would handle all the dialogs.

# Conflicts:
#	libs/angular/src/auth/components/change-password.component.ts
# Conflicts:
#	apps/web/src/app/admin-console/organizations/manage/collections.component.ts
#	apps/web/src/app/admin-console/organizations/manage/groups.component.ts
#	apps/web/src/app/admin-console/organizations/members/components/member-dialog/member-dialog.component.ts
#	apps/web/src/app/admin-console/organizations/shared/components/collection-dialog/collection-dialog.component.ts
#	apps/web/src/app/vault/individual-vault/bulk-action-dialogs/bulk-share-dialog/bulk-share-dialog.component.ts
#	apps/web/src/app/vault/org-vault/vault-header/vault-header.component.ts
#	bitwarden_license/bit-web/src/app/secrets-manager/overview/overview.component.ts
#	bitwarden_license/bit-web/src/app/secrets-manager/settings/porting/sm-import.component.ts
@Hinton Hinton changed the title Draft: Migrate Dialogs to DialogService [PM-1504] Migrate Dialogs to DialogService Mar 23, 2023
@Hinton Hinton marked this pull request as ready for review March 23, 2023 11:16
Copy link
Contributor

@JaredSnider-Bitwarden JaredSnider-Bitwarden left a comment

Choose a reason for hiding this comment

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

Looks great - just a few minor questions.

Thank you for the great work on this! It is so much cleaner than what we had before!

# Conflicts:
#	apps/browser/src/auth/popup/lock.component.ts
#	apps/browser/src/auth/popup/update-temp-password.component.ts
#	apps/browser/src/popup/settings/settings.component.ts
#	apps/browser/src/tools/popup/send/send-add-edit.component.ts
#	apps/browser/src/tools/popup/send/send-groupings.component.ts
#	apps/browser/src/tools/popup/send/send-type.component.ts
#	apps/browser/src/tools/popup/settings/export.component.ts
#	apps/browser/src/vault/popup/components/vault/add-edit.component.ts
#	apps/desktop/src/app/accounts/settings.component.ts
#	apps/desktop/src/app/services/services.module.ts
#	apps/desktop/src/app/tools/export/export.component.ts
#	apps/desktop/src/app/tools/send/add-edit.component.ts
#	apps/desktop/src/app/tools/send/send.component.ts
#	apps/desktop/src/auth/lock.component.ts
#	apps/desktop/src/auth/update-temp-password.component.ts
#	apps/desktop/src/vault/app/vault/add-edit.component.ts
#	apps/web/src/app/admin-console/organizations/manage/collections.component.ts
#	apps/web/src/app/admin-console/organizations/tools/import-export/org-import.component.ts
#	apps/web/src/app/settings/change-kdf/change-kdf.component.ts
#	apps/web/src/app/settings/preferences.component.ts
#	apps/web/src/app/tools/import-export/export.component.ts
#	apps/web/src/app/tools/send/add-edit.component.ts
#	apps/web/src/app/tools/send/send.component.ts
#	apps/web/src/app/vault/individual-vault/add-edit.component.ts
#	apps/web/src/app/vault/individual-vault/vault-items.component.ts
#	apps/web/src/app/vault/org-vault/add-edit.component.ts
#	apps/web/src/app/vault/org-vault/vault-header/vault-header.component.ts
#	apps/web/src/app/vault/org-vault/vault-items.component.ts
#	apps/web/src/app/vault/org-vault/vault.component.ts
#	apps/web/src/auth/lock.component.ts
#	apps/web/src/auth/settings/emergency-access/emergency-add-edit.component.ts
#	apps/web/src/auth/settings/two-factor-base.component.ts
#	apps/web/src/auth/update-temp-password.component.ts
#	bitwarden_license/bit-web/src/app/secrets-manager/secrets/dialog/secret-dialog.component.ts
#	libs/angular/src/auth/components/lock.component.ts
#	libs/angular/src/auth/components/update-temp-password.component.ts
#	libs/angular/src/tools/send/add-edit.component.ts
#	libs/angular/src/tools/send/send.component.ts
#	libs/angular/src/vault/components/add-edit.component.ts
@Hinton Hinton requested review from a team as code owners April 20, 2023 11:54
Copy link
Contributor

@JaredSnider-Bitwarden JaredSnider-Bitwarden left a comment

Choose a reason for hiding this comment

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

LGTM! Great work on this! 👍

# Conflicts:
#	apps/browser/src/services/browserPlatformUtils.service.ts
#	apps/desktop/src/app/app.component.ts
#	apps/web/src/app/app.component.ts
#	bitwarden_license/bit-web/src/app/secrets-manager/projects/project/project.component.ts
djsmith85
djsmith85 previously approved these changes Apr 27, 2023
Copy link
Contributor

@djsmith85 djsmith85 left a comment

Choose a reason for hiding this comment

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

Very nice! Approving files owned by team-tools-dev

Copy link
Contributor

@JaredSnider-Bitwarden JaredSnider-Bitwarden left a comment

Choose a reason for hiding this comment

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

LGTM!

# Conflicts:
#	apps/desktop/src/app/accounts/settings.component.ts
#	apps/web/src/app/settings/preferences.component.ts
#	bitwarden_license/bit-web/src/app/secrets-manager/secrets/dialog/secret-dialog.component.ts
Copy link
Contributor

@Thomas-Avery Thomas-Avery left a comment

Choose a reason for hiding this comment

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

Looks good for SM owned files

@Hinton Hinton removed the needs-qa Marks a PR as requiring QA approval label May 2, 2023
@github-actions github-actions bot added the needs-qa Marks a PR as requiring QA approval label May 2, 2023
@Hinton Hinton merged commit 4e18676 into master May 2, 2023
@Hinton Hinton deleted the ps/dialogs branch May 2, 2023 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-qa Marks a PR as requiring QA approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants