[EC-556] refactor cl button

[coroiu]

Type of change

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

Objective

1. Convert bitButton from Directive to Component to enable template functionality
2. Move loading styling and logic to button component

Code changes

• file.ext: Description of what was changed and why

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

[Hinton]

Could we split this into two PRs?

Re buttons:

What are the motivations for having a regular button with the loading effect? Could this be achieved by having a spinner directive inside a normal button, or is this something that motivates making this toggle-able for all buttons?

Would we be better of having a new directive with the loading style to avoid making the regular button more complicated? You could use it like <button bitButton bitLoadingButton [loading]="true">Delete</button>.

--

Re submit:

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.

[coroiu]

I'm gonna split this into two PRs so I'm only gonna answer the first questions here:

What are the motivations for having a regular button with the loading effect?

Mostly because how I interpreted the design, though I see now that I have not follow the correct loading styles:

Could this be achieved by having a spinner directive inside a normal button, or is this something that motivates making this toggle-able for all buttons?

Absolutely, though that would mean that we duplicate the disabled = attr.disabled || loading logic. Not the end of the world though.

Would we be better of having a new directive with the loading style to avoid making the regular button more complicated? You could use it like <button bitButton bitLoadingButton [loading]="true">Delete.

Mostly because of technical limitations since directives aren't really supposed to be modifying the template. So this only works because I converted the bitButton directive into a component. Though we could implement bitLoadingButton as a component if I convert bitButton back into a directive.

Edit: I see now that the disabled state does not match figma either

[coroiu]

If I'm being honest the main reason for me making these changes were because I thought they were necessary to implement https://github.com/bitwarden/clients/pull/3548. But now that I've thought about it I'm confident that functionality could be implemented in another way if we don't want to add the spinner/loading state to the button.

Though I'm still unsure if we want to mix the loading state with type=submit. There might be times when we don't actually have a form and still want to show loading e.g. a delete button, refresh button, etc``

[Hinton]

The disabled states were changed recently, but the figma is updated on my side.

[coroiu]

@Hinton it's already being used in a few places, though we could replace them. Either way I think I really need to get these PRs merged, what would the QA testing impact be if I replaced the submt-button?

[Hinton]

Impact would be to have QA verify the places we use it still works.