[CL-298] async-actions 2.0: Context string #9396

coroiu · 2024-05-28T13:35:24Z

🎟️ Tracking

CL-298 async-actions 2.0: Context string

📔 Objective

This PR adds more flexibilty to async-actions by allowing developers to create arbitrary groups of async-actions buttons. Examples include:

Grouping buttons in a popover menu
Disabling all buttons in a table when refreshing

It is the second attempt at improving async-actions based on @willmartian suggesting an interesting way of simplifying the context handling, it's more manual but also much less complex:

<bit-table>
  <ng-container body>
    <tr>
      <td>
        <button [bitAction]="foo" context="secrets-table"></button>
        <button [bitAction]="bar" context="secrets-table"></button>
        <button [bitAction]="baz"></button>
      </td>
    </tr>
  </ng-container>
</bit-table>

📸 Screenshots

Everything looks and works the same

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Protected functional changes with optionality (feature flags)
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 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

codecov · 2024-05-28T13:41:38Z

Codecov Report

Attention: Patch coverage is 29.47368% with 67 lines in your changes are missing coverage. Please review.

Project coverage is 28.09%. Comparing base (81c1456) to head (b88fdb4).
Report is 27 commits behind head on main.

❗ Current head b88fdb4 differs from pull request most recent head 921eaca

Please upload reports for the commit 921eaca to get more accurate results.

Files	Patch %	Lines
...mponents/src/async-actions/bit-action.directive.ts	0.00%	23 Missing ⚠️
...mponents/src/async-actions/bit-submit.directive.ts	0.00%	19 Missing ⚠️
...ponents/src/async-actions/form-button.directive.ts	0.00%	13 Missing ⚠️
...components/src/async-actions/standalone.stories.ts	0.00%	4 Missing ⚠️
...ponents/src/async-actions/async-actions.service.ts	90.32%	0 Missing and 3 partials ⚠️
...nents/src/async-actions/async-context.directive.ts	0.00%	3 Missing ⚠️
...mponents/src/async-actions/async-actions.module.ts	0.00%	1 Missing ⚠️
...sync-actions/async-context-provider.abstraction.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9396      +/-   ##
==========================================
+ Coverage   28.05%   28.09%   +0.04%     
==========================================
  Files        2391     2398       +7     
  Lines       70521    70624     +103     
  Branches    13207    13216       +9     
==========================================
+ Hits        19782    19840      +58     
- Misses      49174    49211      +37     
- Partials     1565     1573       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-05-28T13:50:49Z

Checkmarx One – Scan Summary & Details – 12485cd0-f7d9-401b-a5ac-58527985dc62

No New Or Fixed Issues Found

coroiu · 2024-05-29T09:24:44Z

~~TODO: Update documentation~~

Fixed!

coroiu · 2024-05-30T07:14:01Z

libs/components/src/async-actions/async-context.directive.ts

+import { Directive, Input } from "@angular/core";
+
+import { AsyncContextProvider } from "./async-context-provider.abstraction";
+
+@Directive({
+  selector: "[bitAction][context]",
+  providers: [{ provide: AsyncContextProvider, useExisting: BitAsyncContextDirective }],
+})
+export class BitAsyncContextDirective implements AsyncContextProvider {
+  @Input({ required: true })
+  context: string;
+}


suggestion: Maybe "Async Group" would be a better name? Might clash with the form group concept

<div> <button [bitAction]="delete" group="vault-table">Delete item</button> ... <div>

or maybe

<div> <button [bitAction]="delete" asyncGroup="vault-table">Delete item</button> ... <div>

or

<div> <button [bitAction]="delete" actionGroup="vault-table">Delete item</button> ... <div>

Yeah, I would like group or actionGroup if it didn't collide with formGroup. I don't dislike context though--maybe we can see what others think?

coroiu · 2024-05-30T11:19:56Z

libs/components/src/async-actions/async-actions.mdx

+{/* TODO: Add ButtonLikeAbstraction to bitMenuButton */}
+{/* <Story of={stories.GroupedInMenu} /> */}


Need some help with this, the component has changed since I last worked with it

willmartian

I generally love the changes here. I still need to take a deeper look at AsyncActionsService, but some initial thoughts:

willmartian · 2024-05-31T23:13:24Z

libs/components/src/async-actions/async-actions.mdx

+
+# Async Actions


Could you add an import example above the title?

example: https://github.com/bitwarden/clients/blob/main/libs/components/src/item/item.mdx?plain=1#L6-L11

renders as: https://components.bitwarden.com/?path=/docs/component-library-item--docs

willmartian · 2024-05-31T23:15:30Z

libs/components/src/async-actions/async-actions.mdx

+These directives replace the older `appApiAction` directive, providing the option to use
+`observables` and reduce clutter inside our view `components`.


Could you add a jsdoc @deprecated tag to appApiAction and link to this documentation?

willmartian · 2024-05-31T23:22:53Z

libs/components/src/async-actions/bit-action.directive.ts


-  disabled = false;
+  private state$!: Observable<State>;

  @Input("bitAction") handler: FunctionReturningAwaitable;


Could you add a JSdoc comment on this input that describes it and explicitly mentions the arrow function requirement?

willmartian · 2024-05-31T23:53:00Z

libs/components/src/async-actions/async-actions.mdx

+```html
+<form [formGroup]="formGroup" [bitSubmit]="handler">
+  <button bitButton bitFormButton type="submit">Submit</button>
+</form>
+```


🌱 🤔 You know, I wonder how ergonomic this really is when compared to:

<form [formGroup]="formGroup"> <button bitButton [bitAction]="submit" context="my-form" type="submit">Submit</button> </form>

Maybe less ways to do the same thing is better? It feels a bit odd that submit actions are on the form vs the button.

(I wouldn't want to deprecate anything in this PR, just curious what you think.)

I'm actually thinking the opposite, putting the submit action on the button feels odd to me 😄 Here are some reasons why:

Regular angular forms have the submit event on the form

<form [formGroup]="formGroup" (ngSubmit)="onSubmit()"> ... <button type="submit">Submit</button> </form>

Most likely an edge case, but HTML does support multiple submit buttons:

<form [formGroup]="formGroup" (ngSubmit)="onSubmit()"> ... <button type="submit">Submit 1</button> <button type="submit">Submit 2</button> <button type="submit">Submit 3</button> </form>

No submit buttons are also supported by using the "enter" key (not the most accessible design but still)

<form [formGroup]="formGroup" (ngSubmit)="onSubmit()"> ... </form>

willmartian · 2024-05-31T23:56:19Z

libs/components/src/async-actions/async-actions.mdx

+```html
+<button bitButton [bitAction]="handler" context="emergency-contact">Accept</button>
+
+<button bitButton [bitAction]="handler" context="emergency-contact">Reject</button>
+```


Would it make sense to also support passing a template reference variable to context? Naming things is hard, and this gets around that (also prevents collisions by default)

<div #myDiv> <button bitButton [bitAction]="handler" [context]="myDiv">Accept</button> <button bitButton [bitAction]="handler" [context]="myDiv">Reject</button> </div>

Hmmm, yeah sure, as long you can compare them by reference 🤷

But then again, if you've already named the template reference then you could just reuse that name as a string

<div #myDiv> <button bitButton [bitAction]="handler" context="myDiv">Accept</button> <button bitButton [bitAction]="handler" context="myDiv">Reject</button> </div>

willmartian · 2024-06-01T00:56:43Z

libs/components/src/async-actions/async-context.directive.ts

❓ Why split this into a distinct directive? Why not just make context an input on BitActionDirective?

willmartian · 2024-06-01T01:52:38Z

libs/components/src/async-actions/form-button.directive.ts

🎨 This implementation now feels redundant with the addition of context.

Is anything stopping us from updating this to:

@Directive({ selector: "button[bitFormButton]", hostDirectives: [BitActiveDirective], }) export class BitFormButtonDirective { context = inject(BitSubmitDirective).context; }

(I haven't really used hostDirectives before, I could be making an incorrect assumption about how it works.)

Hmm, not sure I follow, what is redundant? You still need something to bind to regular buttons (e.g. cancel) and submit buttons (given that we don't move bitSubmit to them)

coroiu added 6 commits May 28, 2024 10:35

[CL-298] feat: add context story

7a6d78d

[CL-298] feat: add context provider directive

9b7b137

[CL-298] feat: scaffold async actions service

bd917f8

[CL-298] feat: implement async actions service

db01190

[CL-298] feat: add error handling

9f01415

[CL-298] feat: migrate bit action directive to new service

ce6185a

github-actions bot added the needs-qa Marks a PR as requiring QA approval label May 28, 2024

coroiu added 3 commits May 29, 2024 10:37

[CL-298] feat: re-implement bitSubmit

b04706d

[CL-298] chore: some renaming

7f57bc3

[CL-298] feat: add duplicate execution protection

b88fdb4

bitwarden deleted a comment from will May 29, 2024

coroiu commented May 30, 2024

View reviewed changes

[CL-298] feat: update and modernize docs

921eaca

coroiu commented May 30, 2024

View reviewed changes

coroiu marked this pull request as ready for review May 30, 2024 11:42

coroiu requested a review from a team as a code owner May 30, 2024 11:42

willmartian reviewed Jun 1, 2024

View reviewed changes

		{/* TODO: Add ButtonLikeAbstraction to bitMenuButton */}
		{/* <Story of={stories.GroupedInMenu} /> */}

		These directives replace the older `appApiAction` directive, providing the option to use
		`observables` and reduce clutter inside our view `components`.

[CL-298] async-actions 2.0: Context string #9396

Are you sure you want to change the base?

[CL-298] async-actions 2.0: Context string #9396

Uh oh!

Conversation

coroiu commented May 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎟️ Tracking

📔 Objective

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

Uh oh!

codecov bot commented May 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented May 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

No New Or Fixed Issues Found

Uh oh!

coroiu commented May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coroiu May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willmartian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willmartian May 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coroiu Jun 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coroiu commented May 28, 2024 •

edited

Loading

codecov bot commented May 28, 2024 •

edited

Loading

github-actions bot commented May 28, 2024 •

edited

Loading

coroiu commented May 29, 2024 •

edited

Loading

coroiu May 30, 2024 •

edited

Loading

willmartian May 31, 2024 •

edited

Loading

coroiu Jun 3, 2024 •

edited

Loading