From 53e314a517b3b8a67ad5a76e65de37544f448bd2 Mon Sep 17 00:00:00 2001 From: William Martin Date: Mon, 9 Oct 2023 11:23:57 -0400 Subject: [PATCH 1/7] wip --- libs/components/src/stories/migration.mdx | 126 ++++++++++++++++++++++ 1 file changed, 126 insertions(+) create mode 100644 libs/components/src/stories/migration.mdx diff --git a/libs/components/src/stories/migration.mdx b/libs/components/src/stories/migration.mdx new file mode 100644 index 00000000000..0d3a06cd740 --- /dev/null +++ b/libs/components/src/stories/migration.mdx @@ -0,0 +1,126 @@ +import { Meta } from "@storybook/addon-docs"; + + + +# Migrating to the Component Library + +You, dear developer, have been tasked with migrating a component to use the CL. What does that +entail? + +## Getting Started + +Before progressing here, please ensure that... + +- You have fully setup your dev environment as described in the + [contributing docs](https://contributing.bitwarden.com/). +- You are familiar with [Angular reactive forms](https://angular.io/guide/reactive-forms). +- You are familiar with [Tailwind](https://tailwindcss.com/docs/utility-first). + +## Background + +The design of Bitwarden is in flux. At the time of writing, the frontend codebase uses a mix of +multiple UI frameworks: Bootstrap, custom "box" styles, and this component library, which is built +on top of Tailwind. In short, the "CL migration" is a move to only use the CL and remove everything +else. + +This is very important work. Centralizing around a shared design system will: + +- improve user experience by utilizing consistent patterns +- improve developer experience by reducing custom complex UI code +- improve dev & design velocity by having a central location to make UI/UX changes that impact the + entire project + +## Success Criteria + +Follow these steps to fully migrate a component. + +### Use Storybook + +Don't recreate the wheel. + +After reviewing a design, consult this Storybook to determine if there is a component built for your +usecase. Don't waste time styling a button or building a popover menu from scratch--we already have +those. + +### Use Tailwind + +Only use Tailwind for styling. No Bootstrap or other custom CSS is allowed. + +This is easy to verify. Bitwarden prefixes all Tailwind classes with `tw-`. If you see a class +without this prefix, it probably shouldn't be there. + +
+ Bad (Bootstrap) + ```html +
+ ``` +
+ +
+ Good (Tailwind) + ```html +
+ ``` +
+ +**Exception:** Icon font classes, prefixed with `bwi`, are allowed. + +
+ Good (Icons) + ```html + + ``` +
+ +### Use Reactive Forms + +The CL has form components that integrate with Angular's reactive forms: `bit-form-field`, +`bitSubmit`, `bit-form-control`, etc. All forms should be migrated from template-drive forms to +reactive forms to make use of these components. Review the form component docs. + +
+ Bad + ```html +
+ ... +
+ ``` +
+ +
+ Good + ```html +
+ ... +
+ ``` +
+ +**Exception:** There may be instances where migrating to a reactive form isn't feasible. Some +components use inheritance to provide different views in web, desktop, and the browser extension. +_If your goal is to only migrate the web version_ of one such component, you may be unable use +reactive forms if the other clients rely on template-driven forms. + +### Dialogs + +. + +## Examples + +The following examples come from accross the Bitwarden codebase. + +### 1 Platform + +https://github.com/bitwarden/clients/pull/6301/files + +### 2 Auth + +https://github.com/bitwarden/clients/pull/5377 + +### 3 AC + +https://github.com/bitwarden/clients/pull/5417 + +### 4 Vault + +https://github.com/bitwarden/clients/pull/5648 From 0f1424c0f90ca11dcc27128c90d89396c605b5a0 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 16 Nov 2023 17:57:57 -0500 Subject: [PATCH 2/7] add dialog docs --- libs/components/src/stories/migration.mdx | 42 +++++++++++++++++++---- 1 file changed, 35 insertions(+), 7 deletions(-) diff --git a/libs/components/src/stories/migration.mdx b/libs/components/src/stories/migration.mdx index 0d3a06cd740..74a199e7edd 100644 --- a/libs/components/src/stories/migration.mdx +++ b/libs/components/src/stories/migration.mdx @@ -81,18 +81,18 @@ reactive forms to make use of these components. Review the form component docs.
Bad ```html -
- ... -
+
+ ... +
```
Good ```html -
- ... -
+
+ ... +
```
@@ -103,7 +103,35 @@ reactive forms if the other clients rely on template-driven forms. ### Dialogs -. +Legacy Bootstrap modals use the `ModalService`. These should be converted to use the `DialogService` +and it's related CL components. Components that are fully migrated should have no reference to the +`ModalService`. + +
+ Bad + ```ts + this.modalService.open(DeleteAccountComponent); + ``` + + ```html + + + ``` + +
+ +
+ Good + ```ts + this.dialogService.open(DeleteAccountComponent); + ``` + + ```html + + ... + ``` + +
## Examples From ac6106b189b0a3f8eebda494b067b76c743b5b37 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 16 Nov 2023 18:29:06 -0500 Subject: [PATCH 3/7] add blurb to example prs --- libs/components/src/stories/migration.mdx | 44 +++++++++++++++++------ 1 file changed, 33 insertions(+), 11 deletions(-) diff --git a/libs/components/src/stories/migration.mdx b/libs/components/src/stories/migration.mdx index 74a199e7edd..2419a46f381 100644 --- a/libs/components/src/stories/migration.mdx +++ b/libs/components/src/stories/migration.mdx @@ -4,8 +4,7 @@ import { Meta } from "@storybook/addon-docs"; # Migrating to the Component Library -You, dear developer, have been tasked with migrating a component to use the CL. What does that -entail? +You have been tasked with migrating a component to use the CL. What does that entail? ## Getting Started @@ -96,11 +95,6 @@ reactive forms to make use of these components. Review the form component docs. ``` -**Exception:** There may be instances where migrating to a reactive form isn't feasible. Some -components use inheritance to provide different views in web, desktop, and the browser extension. -_If your goal is to only migrate the web version_ of one such component, you may be unable use -reactive forms if the other clients rely on template-driven forms. - ### Dialogs Legacy Bootstrap modals use the `ModalService`. These should be converted to use the `DialogService` @@ -137,18 +131,46 @@ and it's related CL components. Components that are fully migrated should have n The following examples come from accross the Bitwarden codebase. -### 1 Platform +### 1.) AboutComponent + +Codeowner: Platform https://github.com/bitwarden/clients/pull/6301/files -### 2 Auth +This migration updates a `ModalService` component to the `DialogService`. + +**Note:** Most of the internal markup of this component was unchanged, aside from the removal of +defunct Bootstrap classes. + +### 2.) Auth + +Codeowner: Auth https://github.com/bitwarden/clients/pull/5377 -### 3 AC +This PR also does some general refactoring, the main relevant change can be seen here: + +[Old template](https://github.com/bitwarden/clients/pull/5377/files#diff-4fcab9ffa4ed26904c53da3bd130e346986576f2372e90b0f66188c809f9284d) +--> +[New template](https://github.com/bitwarden/clients/pull/5377/files#diff-cb93c74c828b9b49dc7869cc0324f5f7d6609da6f72e38ac6baba6d5b6384327) + +Updates a dialog, similar to example 1, but also adds CL form components and Angular Reactive Forms. + +### 3.) AC + +Codeowner: Admin Console https://github.com/bitwarden/clients/pull/5417 -### 4 Vault +Migrates dialog, form, buttons, and a table. + +### 4.) Vault + +Codeowner: Vault https://github.com/bitwarden/clients/pull/5648 + +Some of our components are shared between multiple clients (web, desktop, and the browser extension) +through the use of inheritance. This PR updates the _web_ template of a cross-client component to +use Tailwind and the CL, and updates the base component implementation to use reactive forms, +without updating the desktop or browser templates. From 562cb6cb57b8ad19a5a0a4cd2be80061a140f65d Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 16 Nov 2023 18:42:08 -0500 Subject: [PATCH 4/7] edits --- libs/components/src/stories/migration.mdx | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/libs/components/src/stories/migration.mdx b/libs/components/src/stories/migration.mdx index 2419a46f381..30a92b2253e 100644 --- a/libs/components/src/stories/migration.mdx +++ b/libs/components/src/stories/migration.mdx @@ -39,7 +39,7 @@ Don't recreate the wheel. After reviewing a design, consult this Storybook to determine if there is a component built for your usecase. Don't waste time styling a button or building a popover menu from scratch--we already have -those. +those. If a component isn't flexible enough or doesn't exist for your usecase, contact Will Martin. ### Use Tailwind @@ -174,3 +174,7 @@ Some of our components are shared between multiple clients (web, desktop, and th through the use of inheritance. This PR updates the _web_ template of a cross-client component to use Tailwind and the CL, and updates the base component implementation to use reactive forms, without updating the desktop or browser templates. + +## Questions + +Please direct any development questions to Will Martin. Thank you! From 477ac9055c6f1c7c627094870394a99aad97ff54 Mon Sep 17 00:00:00 2001 From: William Martin Date: Fri, 17 Nov 2023 13:09:48 -0500 Subject: [PATCH 5/7] expand dialog docs --- libs/components/src/stories/migration.mdx | 70 +++++++++++++++++------ 1 file changed, 54 insertions(+), 16 deletions(-) diff --git a/libs/components/src/stories/migration.mdx b/libs/components/src/stories/migration.mdx index 30a92b2253e..1713319e33e 100644 --- a/libs/components/src/stories/migration.mdx +++ b/libs/components/src/stories/migration.mdx @@ -38,8 +38,9 @@ Follow these steps to fully migrate a component. Don't recreate the wheel. After reviewing a design, consult this Storybook to determine if there is a component built for your -usecase. Don't waste time styling a button or building a popover menu from scratch--we already have -those. If a component isn't flexible enough or doesn't exist for your usecase, contact Will Martin. +usecase. Don't waste effort styling a button or building a popover menu from scratch--we already +have those. If a component isn't flexible enough or doesn't exist for your usecase, contact Will +Martin. ### Use Tailwind @@ -75,7 +76,8 @@ without this prefix, it probably shouldn't be there. The CL has form components that integrate with Angular's reactive forms: `bit-form-field`, `bitSubmit`, `bit-form-control`, etc. All forms should be migrated from template-drive forms to -reactive forms to make use of these components. Review the form component docs. +reactive forms to make use of these components. Review the +[form component docs](?path=/docs/component-library-form--docs).
Bad @@ -98,35 +100,71 @@ reactive forms to make use of these components. Review the form component docs. ### Dialogs Legacy Bootstrap modals use the `ModalService`. These should be converted to use the `DialogService` -and it's related CL components. Components that are fully migrated should have no reference to the -`ModalService`. +and it's [related CL components](?path=/docs/component-library-dialogs--docs). Components that are +fully migrated should have no reference to the `ModalService`. -
- Bad - ```ts - this.modalService.open(DeleteAccountComponent); - ``` +1. Update the template to use CL components: +
```html - + ``` +
+
+ ```html + + ... + ```
+2. Create a static `open` method on the component, that calls `DialogService.open`: +
- Good ```ts - this.dialogService.open(DeleteAccountComponent); + export class FooDialogComponent { + //... + + static open(dialogService: DialogService) { + return dialogService.open(DeleteAccountComponent); + } + } ``` - ```html - - ... +
+ +3. If you need to pass data into the dialog, pass it to `open` as a parameter and inject + `DIALOG_DATA` into the component's constructor. + +
+ ```ts + export type FooDialogParams = { + bar: string; + } + + export class FooDialogComponent { + constructor(@Inject(DIALOG_DATA) protected params: FooDialogParams) {} + + static open(dialogService: DialogService, data: FooDialogParams) { + return dialogService.open(DeleteAccountComponent, { data }); + } + } ```
+4. Replace calls to `ModalService.open` or `ModalService.openViewRef` with the newly created static + `open` method: + +
+ ```ts this.modalService.open(FooDialogComponent); // or ModalService.openViewRef ``` +
+ +
+ ```ts FooDialogComponent.open(this.dialogService); ``` +
+ ## Examples The following examples come from accross the Bitwarden codebase. From 0bcaa1f02cdc7eb28f7d7174ca6d3264f3d37edf Mon Sep 17 00:00:00 2001 From: William Martin Date: Fri, 17 Nov 2023 13:28:30 -0500 Subject: [PATCH 6/7] formatting error --- libs/components/src/stories/migration.mdx | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/libs/components/src/stories/migration.mdx b/libs/components/src/stories/migration.mdx index 1713319e33e..d946479607b 100644 --- a/libs/components/src/stories/migration.mdx +++ b/libs/components/src/stories/migration.mdx @@ -158,11 +158,18 @@ fully migrated should have no reference to the `ModalService`. `open` method:
- ```ts this.modalService.open(FooDialogComponent); // or ModalService.openViewRef ``` + ```ts + this.modalService.open(FooDialogComponent); + +````
- ```ts FooDialogComponent.open(this.dialogService); ``` +```ts + FooDialogComponent.open(this.dialogService); + +```` +
## Examples From a2920a2f6d06eedc4f1724ecfc29a1f890fd6d0f Mon Sep 17 00:00:00 2001 From: William Martin Date: Fri, 17 Nov 2023 13:29:47 -0500 Subject: [PATCH 7/7] fix formatting --- libs/components/src/stories/migration.mdx | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/libs/components/src/stories/migration.mdx b/libs/components/src/stories/migration.mdx index d946479607b..730cfdb412f 100644 --- a/libs/components/src/stories/migration.mdx +++ b/libs/components/src/stories/migration.mdx @@ -157,20 +157,9 @@ fully migrated should have no reference to the `ModalService`. 4. Replace calls to `ModalService.open` or `ModalService.openViewRef` with the newly created static `open` method: -
- ```ts - this.modalService.open(FooDialogComponent); - -```` -
+
`this.modalService.open(FooDialogComponent);`
-
-```ts - FooDialogComponent.open(this.dialogService); - -```` - -
+
`FooDialogComponent.open(this.dialogService);`
## Examples