clients
clients copied to clipboard
[EC-558] refactor `appApiAction` (add new alternative)
Type of change
- [ ] Bug fix
- [ ] New feature development
- [x] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other
Objective
Refactor the older appApiAction directive, modernize it to use observables and add it to the CL.
Usage example
Standard method
<form [formGroup]="formGroup" (ngSubmit)="submit()">
<button type="submit" bitButton [loading]="loading">
{{ "submit" | i18n }}
</button>
</form>
class Component {
loading = false;
formGroup = this.formBuilder.group({...})
async submit() {
this.loading = true;
try {
// [... submit logic ...]
} catch (err) {
this.logService.log(err);
} finally {
this.loading = false;
}
}
}
appApiAction
method
<form #form (ngSubmit)="submit()" [appApiAction]="formPromise">
<button type="submit" bitButton [loading]="form.loading">
{{ "submit" | i18n }}
</button>
</form>
class Component {
formPromise: Promise<unknown>;
formGroup = this.formBuilder.group({...})
async submit() {
try {
this.formPromise = this.doSubmit();
await this.formPromise;
} catch (err) {
this.logService.log(err);
}
}
private async doSubmit() {
// [... submit logic ...]
}
}
(New in this PR) bitSubmit
method
<form [formGroup]="formGroup" [bitSubmit]="submit">
<button type="submit" bitButton bitFormButton>
{{ "submit" | i18n }}
</button>
</form>
class Component {
formGroup = this.formBuilder.group({...})
//note: submit can also return Observable instead of Promise
submit: async () => {
try {
// [... submit logic ...]
} catch (err) {
this.logService.log(err);
}
}
}
Code changes
-
bit-submit.directive: New directive that attaches to a
form
tag with[formGroup]
directive. Most of the magic happens here. The directive attaches to a form, extracts theFormGroupDirective
and hooks intongSubmit
. When the user submits,BitSubmitDirective
picks up the action and triggers the developer-supplied async submit handler. The status of the async execution is exposed using the publicloading$
observable field. Note: If the submit handler returns anObservable
it will automatically be unsubscribed and aborted if the UI is destroyed. -
form-button.directive: A layer in-between
bitButton
andbitSubmit
. It subscribes toBitSubmitDirective.loading$
and setsBitButtonDirective.loading
accordingly. This results in the button automatically disabling and showing loading state when the async submit handler is running. -
submit-button.component: A convenient component that can be used as a shorthand for
<button type="submit" bitButton bitFormButton>
{{ "submit" | i18n }}
</button>
Screenshots
See storybook
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
Comment from @Hinton in https://github.com/bitwarden/clients/pull/3537
I think it's a good refactor, it will certainly let us get rid of the annoying form promises we create everywhere. I'm unsure of the
BitFormButtonDirective
, is the thinking that we want to disable all buttons during a form submit? If so we probably don't want all of them to display the loading effect but rather disable them, with the submit button having the effect.
Yeah you're probably right about that, cancel buttons should probably not have the spinner. But if everything else looks good then I'd gladly but some more effort into this and fix that issue!
As we discussed this seems like a nice improvement. We'll want to rework this so that buttons can have their own handlers if they aren't the main submit button. And the other things we discussed like inverting the relationship and have buttons check for the action.
Currently waiting to merge base branch, will then update this and prepare it for review
I do think we need to document this thoroughly. What are your thoughts on adding descriptions to each story + some comments to the directives, and writing a markdown help page https://storybook.js.org/docs/react/writing-docs/mdx#documentation-only-mdx
We should also spend a few minutes thinking through how we want to organize the storybook pages for these components, since they are all inter-connected, and having them be spread out will be confusing.
@Hinton I agree with both of these, I can add the documentation! Do you have any suggestion on the organizational structure? I have no idea what would be better
This is really nice, amazing work! 🎉
Any chance you can swap over an existing use of appApiAction
to these new directives? Maybe one standalone button, and a submit case? Your usage examples are great, but it'd be nice to see these being used in action 😎