fix(select): compatibility with angular forms API

badsgahhl commented 4 months ago

PR Checklist

Please check if your PR fulfills the following requirements:

[x] The commit message follows our guidelines: https://github.com/goetzrobin/spartan/blob/main/CONTRIBUTING.md#-commit-message-guidelines
[ ] Tests for the changes have been added (for bug fixes / features)
[ ] Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[X] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[X] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

Which package are you modifying?

[ ] accordion
[ ] alert
[ ] alert-dialog
[ ] aspect-ratio
[ ] avatar
[ ] badge
[ ] button
[ ] calendar
[ ] card
[ ] checkbox
[ ] collapsible
[ ] combobox
[ ] command
[ ] context-menu
[ ] data-table
[ ] date-picker
[ ] dialog
[ ] dropdown-menu
[ ] hover-card
[ ] input
[ ] label
[ ] menubar
[ ] navigation-menu
[ ] pagination
[ ] popover
[ ] progress
[ ] radio-group
[ ] scroll-area
[X] select
[ ] separator
[ ] sheet
[ ] skeleton
[ ] slider
[ ] spinner
[ ] switch
[ ] table
[ ] tabs
[ ] textarea
[ ] toast
[ ] toggle
[ ] tooltip
[ ] typography

What is the current behavior?

This PR fixes several issues of the select component if its used together with the angular forms API.

What is the new behavior?

Fixing a NG0600 issue if the value of the select is set from a FormControl
Initial values of a FormControl are correctly passed to the select component
Synchronizing the disabled state with the FormControl
Values from the component are correctly passed back to the ReactiveFormsAPI
The value type depends on the multiple property. If it is true, value is going to be a string array otherwise a normal string
TBD: Displaying an error state depending on the FormControl

Does this PR introduce a breaking change?

[ ] Yes
[X] No

alexciesielski commented 4 months ago

This change is much needed, I was gonna start this myself but saw your draft already. Let me know if I can help to get this ready to be merged.

thatsamsonkid commented 4 months ago

@badsgahhl - thanks for helping out in trying to fix some of these form issues with Select. I know this is still in draft but I think most of the changes are looking good so far. Only thing I would comment on if its at all possible to avoid the write signals in the effect I think that would be best, but otherwise this is coming along great. As @alexciesielski mentioned let us know if we can do anything to help get this merged!

goetzrobin commented 4 months ago

@badsgahhl - thanks for helping out in trying to fix some of these form issues with Select. I know this is still in draft but I think most of the changes are looking good so far. Only thing I would comment on if its at all possible to avoid the write signals in the effect I think that would be best, but otherwise this is coming along great. As @alexciesielski mentioned let us know if we can do anything to help get this merged!

@thatsamsonkid do you think using untracked is better or are you in general saying that this might cause infinite loops?

thatsamsonkid commented 4 months ago

@badsgahhl - thanks for helping out in trying to fix some of these form issues with Select. I know this is still in draft but I think most of the changes are looking good so far. Only thing I would comment on if its at all possible to avoid the write signals in the effect I think that would be best, but otherwise this is coming along great. As @alexciesielski mentioned let us know if we can do anything to help get this merged!

@thatsamsonkid do you think using untracked is better or are you in general saying that this might cause infinite loops?

I was just saying in general. Technically it shouldn't have infinite loop issue since I believe listbox change here will now only update the value in the service and then the effect will see that has changed and will update value in root component and trigger change. It should be ok, but I think my preference would be to avoid allowSignalWrite if possible if not, I'm good.

badsgahhl commented 4 months ago

I pushed two commits to improve the mentioned things. I just removed the inner value of the brn-select.component so the whole state is now fully handled by the service. The effect is now only needed to sync the state value back to the forms api.

Otherwise I could need some inspiration of the error state. As I understand the Forms API and already debugged it, Angular places the ng-invalid and ng-touched classes onto the element that is used by the Forms API. In this case those classes are on the hlm-select element. However, the trigger button is three layers below so I really don't know how to make the border red etc. if the brn-select Component is invalid. If anyone has an idea how to do this?

thatsamsonkid commented 4 months ago

I pushed two commits to improve the mentioned things. I just removed the inner value of the brn-select.component so the whole state is now fully handled by the service. The effect is now only needed to sync the state value back to the forms api.

Otherwise I could need some inspiration of the error state. As I understand the Forms API and already debugged it, Angular places the ng-invalid and ng-touched classes onto the element that is used by the Forms API. In this case those classes are on the hlm-select element. However, the trigger button is three layers below so I really don't know how to make the border red etc. if the brn-select Component is invalid. If anyone has an idea how to do this?

I think we could probably get that information from the ngControl, can track the statuses as state in the service and then access those values in the trigger.

alexciesielski commented 4 months ago

You can inject NgControl in the constructor and check it's valid property. Or use the statusChanges Observable, depending on how you want to access it.

constructor(
   @Optional() @Self() public ngControl: NgControl | null
) {}

// ...

const isValid = this.ngControl ? this.ngControl.control.valid : true; // defaulting to `valid` if no ngModel/formControl passed

wizardnet972 commented 4 months ago

Please take into consideration the problem highlighted in this issue and the demonstration provided in the StackBlitz regarding the select control:

Issue link: https://github.com/goetzrobin/spartan/issues/188#issuecomment-1979262542

StackBlitz demonstration: https://stackblitz.com/edit/github-s5zire?file=src%2Fmain.ts

goetzrobin commented 4 months ago

@badsgahhl let me know if there's anything else we can do to help you get this ready for merging! Thanks for your work on this

badsgahhl commented 4 months ago

You can inject NgControl in the constructor and check it's valid property. Or use the statusChanges Observable, depending on how you want to access it.
constructor(
   @Optional() @Self() public ngControl: NgControl | null
) {}

// ...

const isValid = this.ngControl ? this.ngControl.control.valid : true; // defaulting to `valid` if no ngModel/formControl passed

One thing I noticed here is that ngControl.control is null during the constructor and gets populated later, therefore I can't access the status changes Observable. Will try to find a solution for that, that's the only thing that makes me a headache.

thatsamsonkid commented 4 months ago

@badsgahhl - Did you try removing @Self, I think its not needed here. I'd be a little surprised if it still didn't resolve after that because ti should be assigned by the time the trigger is created.

badsgahhl commented 4 months ago

Got it. Should be done now.

thatsamsonkid commented 4 months ago

Ok just for awareness i'll look into making some tests for this hopefully tonight. Thanks!

goetzrobin commented 4 months ago

Awesome @thatsamsonkid I was going to wait for this PR to release the next alpha, but we can also do a follow up PR of tests and I'll try to get this out first thing tomorrow. Since the new release should improve a lot of little pains

thatsamsonkid commented 4 months ago

@goetzrobin - so there is a few issues I found now that I played around with it and writing the tests.

I fixed some of the issues and added some tests - see https://github.com/badsgahhl/spartan/pull/2

However I did find the following:

cdkListbox not aware of initial form value:

Both now and even with my changes, setting an initial value, cdklistbox is unaware an option has been preselected. All of our components know about it initially but actual cdkoption aria attributes will show it as unselected

Both now and even with my pr, If select is set with initial value and switched to multiple, select will keep displaying initial value along with additional options but actual form control only has the options selected afterwards listed.

The error state doesn't match shadcn design, I was actually surprised but it seems only label and error message actually turn red and not the actual select trigger. Attaching for reference
Also this branch needs to be rebased as well, saw some outdated code mainly just with the select stories

Things I fixed in the reverse PR:

ng-pristine and untouched
initial value was triggering valueChange making select always in the dirty state and never pristine
tests for the ng-form-classes

But we can definitely deal with these other issues afterwards if you like. To be honest not sure if I caused any other side effects with my changes also in that PR so if you did want to release I would maybe recommend just with these changes for now. Maybe just some minor relief for some pain points.

thatsamsonkid commented 4 months ago

Just providing an update, I pushed a fix for the styling to match the shadcn error state and I also fixed the issue with setting the initial values on cdkListbox if anyone wants to check that out. I think there's a few more tests I could write to ensure these initial value scenarios but I think at least my pr solves all the issues I found above for the moment.

One thing I was seeing, I think I will add this in a different PR but we can also add a brn-error/hlm-error directive similar to our label directive so we can fully match the error state style

thatsamsonkid commented 4 months ago

Alright added tests for all those scenarios. @badsgahhl if you want to take a look at that PR I have in your fork if its all good if you could merge it so it can be part of this PR and then I think we can do a final code review here!

badsgahhl commented 4 months ago

Sorry for taking so long, looked into the things and they look good. Thanks for the testing!

Only one e2e test is currently failing in my setup:

────────────────────────────────────────────────────────────────────────────────────────────────────

  Running:  select/select.cy.ts                                                           (16 of 27)

  select
    default
      1) click on trigger should open and close it content
      √ should close after selecting an option in single mode (749ms)
    multiple
      √ should stay open after selecting an option in multi mode (950ms)
      √ should stay open after selecting an option in multi mode (945ms)
    disabled
      √ should not open if disabled (448ms)
    form validation
      √ should have form validation classes and reflect control status (987ms)
      √ should have form validation classes and reflect control status with label (923ms)
      √ should have form validation classes and reflect control status when assigned with initial value (978ms)
      √ should have form validation classes and reflect control status (757ms)
    scrollable
      √ should be scrollable (1209ms)
    scrollable with sticky headers
      √ should be scrollable with sticky headers (1029ms)

  10 passing (14s)
  1 failing

  1) select
       default
         click on trigger should open and close it content:
     AssertionError: Timed out retrying after 4000ms: expected '<button#brn-select-0--trigger.bg-background.border.border-input.disabled:cursor-not-allowed.disabled:opacity-50.flex.focus:outline-none.focus:ring-2.focus:ring-offset-2.focus:ring-ring.h-10.items-center.justify-between.placeholder:text-muted-foreground.px-3.py-2.ring-offset-background.rounded-md.text-sm.w-56>' to be 'focused'
      at verifySelectSetup (webpack:///./src/integration/select/select.cy.ts:50:0)
      at Context.eval (webpack:///./src/integration/select/select.cy.ts:59:0)

  (Results)

  ┌────────────────────────────────────────────────────────────────────────────────────────────────┐
  │ Tests:        11                                                                               │
  │ Passing:      10                                                                               │
  │ Failing:      1                                                                                │
  │ Pending:      0                                                                                │
  │ Skipped:      0                                                                                │
  │ Screenshots:  1                                                                                │
  │ Video:        false                                                                            │
  │ Duration:     14 seconds                                                                       │
  │ Spec Ran:     select/select.cy.ts                                                              │
  └────────────────────────────────────────────────────────────────────────────────────────────────┘

Will try to fix that, but its reviewable in the meantime.

thatsamsonkid commented 4 months ago

No worries and sure thing! Thats weird that last test failed for you, all the tests were passing for me before with my last commit. I kicked off the CI job here. Lets see what the results are here.

thatsamsonkid commented 4 months ago

@badsgahhl - I think we're good seems everything passed here. Just requesting a final review and we should probably be good to merge!

goetzrobin commented 4 months ago

LGTM!

jon9090 commented 4 months ago

Inquiring whether it's feasible to implement a feature where multiple modes can be checked and unchecked, but changes are only applied upon clicking the "OK" button. Conversely, if the "Cancel" or close button is clicked, no modifications should take effect. As the component's author, is this achievable within the existing framework, or would it necessitate creating a custom version of the select functionality?

goetzrobin commented 4 months ago

Inquiring whether it's feasible to implement a feature where multiple modes can be checked and unchecked, but changes are only applied upon clicking the "OK" button. Conversely, if the "Cancel" or close button is clicked, no modifications should take effect. As the component's author, is this achievable within the existing framework, or would it necessitate creating a custom version of the select functionality?

Are you saying you want a separate dialog that allows you to toggle between a select components multiple or single mode or that you'd like to have the user select multiple values in the select and then only apply them after they click OK?

badsgahhl commented 3 months ago

Would it be possible to make a new release of the select package? @goetzrobin

goetzrobin / spartan