goetzrobin / spartan

Cutting-edge tools powering Angular full-stack development.
https://spartan.ng
MIT License
1.13k stars 123 forks source link

Why is the value of the select component wrapped in an array when the 'multiple' property is not present? #188

Closed wizardnet972 closed 4 months ago

wizardnet972 commented 4 months ago

Please provide the environment you discovered this bug in.

Storybook and the code itself

Which area/package is the issue in?

select

Description

Why is the value of the select component wrapped in an array when it does not contain the multiple property?

image

Please provide the exception or error you saw

No response

Other information

No response

I would be willing to submit a PR to fix this issue

thatsamsonkid commented 4 months ago

Hi, seems there is a few issues with the Select form compatibility that have been uncovered. I think both this issue and the other issue you raised #189 will be resolved with this PR #184

wizardnet972 commented 4 months ago

@thatsamsonkid Yes, I replaced those files and now the select has its initial value set as an object instead of an array. Could you please merge and publish this change?

wizardnet972 commented 4 months ago

@thatsamsonkid, there's another issue with the select functionality. (I apply your changes)

When I switch from having multiple selections to a single one in the select option, and then select a value, and then switch back to multiple selections, I find that I'm unable to select anything anymore.

Below is the code snippet that binds to the selects:

  durations = {
    daily: [
      { label: 'Morning', value: 'morning' },
      { label: 'Afternoon', value: 'afternoon' },
    ],
    weekly: [
      { label: 'Monday', value: 'monday' },
      { label: 'Tuesday', value: 'tuesday' },
      { label: 'Wednesday', value: 'wednesday' },
      { label: 'Thursday', value: 'thursday' },
      { label: 'Friday', value: 'friday' },
    ],
    monthly: [
      { label: 'Start of Month', value: 'startOfMonth' },
      { label: 'End of Month', value: 'endOfMonth' },
    ],
    quarterly: [
      { label: 'Start of Quarter', value: 'startOfQuarter' },
      { label: 'End of Quarter', value: 'endOfQuarter' },
    ],
  };
  frequencies = Object.keys(this.durations).map((k) => ({ label: upperFirst(k), value: k.toLowerCase() }));

  reports = [{ name: 'Clients', frequency: null, duration: null, active: true }];

The frequencies select influences the duration select. When the frequencies select has the value of daily, the duration select transforms into a multiselect.

Here's the scenario:

  1. Select daily in the frequencies select.
  2. Choose two values in the duration select.
  3. Change to weekly in the frequencies select.
  4. Pick one value in the duration select.
  5. Switch back to daily in the frequencies select.
  6. Attempt to select values in the duration select (unable to).

The frequencies select:

   <hlm-select class="inline-block" [(ngModel)]="report.frequency" placeholder="Select frequency">
                    <hlm-select-trigger class="w-56">
                      <hlm-select-value />
                    </hlm-select-trigger>
                    <hlm-select-content class="w-56">
                      <hlm-option *ngFor="let frequency of frequencies" [value]="frequency.value">{{ frequency.label }}</hlm-option>
                    </hlm-select-content>
                  </hlm-select>

The duration select:

                  <hlm-select
                    class="inline-block"
                    placeholder="Select time/duration"
                    [multiple]="report.frequency === 'daily'"
                    [(ngModel)]="report.duration">
                    <hlm-select-trigger class="w-56">
                      <hlm-select-value />
                    </hlm-select-trigger>
                    <hlm-select-content class="w-56">
                      <hlm-option *ngFor="let duration of durations[report.frequency]" [value]="duration.value">
                        {{ duration.label }}
                      </hlm-option>
                    </hlm-select-content>
                  </hlm-select>
wizardnet972 commented 4 months ago

@thatsamsonkid and @goetzrobin, I'm currently investigating the source code of the select and Angular CDK. It appears that you're utilizing listbox to retrieve all the checked items. However, it seems that this approach isn't functioning as expected in the above scenario. Specifically, when it transitions to 'multiple' mode, the listbox retains values even when they are unchecked.

Therefore, I propose a straightforward solution: upon a mode change (such as transitioning to 'multiple'), clear all selected items as follows:

_cdkListbox.selectionModel.selected.forEach(value => {
   _cdkListbox.selectionModel.deselect(value)
});

It appears that Angular/CDK doesn't effectively clear the items in such scenarios.

thatsamsonkid commented 4 months ago

So I think that was intentional by the material team. They only clear if you go from multiple -> single because there's no way to know which option to keep if multiple are selected.

If you go from single to multiple there is no issue there since your just allowing to add additional options. I think from a default behavior I think this is fine, but I do believe if you have a use case where it does need to be cleared then since I assume you are programmatically changing the mode anyways then you can also just clear the form field through your form control I believe as well.

That PR is still being finalized, its also not might PR, i'm just a reviewer for that one but we are definitely determined to get these fixes in as soon as possible!

wizardnet972 commented 4 months ago

@thatsamsonkid

I've demonstrated the issue here, and it's not tied to the form model. In the provided example, there's no form model involved.

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

  1. Begin by selecting "Weekly" for the frequency, then choose "Monday" for the duration.
  2. Next, switch the frequency to "Daily" and select "Morning" for the duration. You'll observe that this selection doesn't function as expected.

I've reproduced the issue using select source files and implemented changes from the https://github.com/goetzrobin/spartan/pull/184 PR by substituting the brain source code with three modifications from the PR.

Also the datasource can be changes according to the first select option. so it might now have the value from multiple or single select.

The easy solution is to deselect all the options when the option is initialize. both, the options in the state and the options inside the Listbox:

export class BrnSelectOptionDirective ... {
   .....
  constructor() {
    this._selectService.deselectAllOptions();
    (this._cdkSelectOption as any).listbox.selectionModel.selected.forEach((selected) => {
      (this._cdkSelectOption as any).listbox.selectionModel.deselect(selected);
    });

    this._selectService.registerOption(this._cdkSelectOption);

    toObservable(this._selectService.value)
    .pipe(takeUntilDestroyed())
     .......

The cdk doesn't expose the listbox but it exist in the js object.

jon9090 commented 4 months ago

+1. The issue is identical for me.

thatsamsonkid commented 4 months ago

Hmm ok thanks for putting that example together. Maybe what we could do is add an input/flag "clearOnModeChange" to enforce clearing regardless of what mode is transitioning to. Im a little hesitant to make it the default behavior. I believe the intention behind that default behavior is to be the least destructive to data where possible. @goetzrobin @elite-benni any thoughts on this one?

wizardnet972 commented 4 months ago

@thatsamsonkid I understand your point. However, given that the selection-state resides within the "brain" package, directly overriding the default logic might not be feasible. Perhaps we could consider adding an additional input on the "ui" side, which would then need to communicate with the service responsible for managing operations within the "brain" package. Whether or not this override is possible would depend on whether the necessary functionalities are exposed for such interactions. Ultimately, the logic governing the selection process should remain primarily on the "brain" side for coherence and consistency.

elite-benni commented 4 months ago

Hmm ok thanks for putting that example together. Maybe what we could do is add an input/flag "clearOnModeChange" to enforce clearing regardless of what mode is transitioning to. Im a little hesitant to make it the default behavior. I believe the intention behind that default behavior is to be the least destructive to data where possible. @goetzrobin @elite-benni any thoughts on this one?

I totally understand both points. In the example all options also changed, and I can understand that in this case the value should just be cleared, even if it is a change from single to multiple. But if the options stay the same, and there is only one selected, it should remain selected.