fix(select): update initial value properly when using multi-mode and …

guillermoecharri commented 1 month ago

…options are added with for-loop

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
[x] Tests for the changes have been added (for bug fixes / features)
[x] 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)
[ ] 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
[ ] icon
[ ] input
[ ] label
[ ] menubar
[ ] navigation-menu
[ ] pagination
[ ] popover
[ ] progress
[ ] radio-group
[ ] scroll-area
[x] select
[ ] separator
[ ] sheet
[ ] skeleton
[ ] slider
[ ] sonner
[ ] spinner
[ ] switch
[ ] table
[ ] tabs
[ ] textarea
[ ] toast
[ ] toggle
[ ] tooltip
[ ] typography

What is the current behavior?

Currently, in the select component, if you use multi-mode, provide an initial value, and dynamically add options in the template (e.g., using a for-loop), the initial value is not applied correctly. Although it initially appears to be set correctly, selecting an option reveals that no initial value was actually applied. Additionally, the component displays as if both the initial value and the selected option are active, even showing duplicates.

Closes #

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

guillermoecharri commented 1 month ago

Hi there! First time contributor here so sorry if I messed anything up. I see that I have some issues with "ci / prettier" and "UI Tests". Those seem to be failing on main as well so I'm not sure if I need to do something to fix those on my end. Also not sure if I need to do anything to pass the Vercel check.

thatsamsonkid commented 1 month ago

Hi there! First time contributor here so sorry if I messed anything up. I see that I have some issues with "ci / prettier" and "UI Tests". Those seem to be failing on main as well so I'm not sure if I need to do something to fix those on my end. Also not sure if I need to do anything to pass the Vercel check.

Hi @guillermoecharri like you mentioned seems these are some existing issues in main and not related to the select component so I wouldn't worry about it. At first glance I think this LGTM to me and seems all select tests are still passing. Really liking the change actually but want to give a chance for maybe @elite-benni to take a quick look in case I'm not considering something here

Thanks for taking the time to contribute!

guillermoecharri commented 1 month ago

First of all, thanks for controbuting.

To me this looks like another tricky scenario that initially the possibleOptionsChanged function in the brain service tried to solve.

I am not sure why this does not work, because this was also introduced to solve an issue with options added in a for loop.

I would like to check why the old solution does not work in that case, but unfortunately i only have access to my phone right now.

The change looks good to me tough.

Yeah it's a very tricky timing issue. If you want to debug the specific scenario when you have time you can comment-out lines 322-327 and check out the new story that I added. When setInitialSelectedOptions gets called inside writeValue, it won't be able to access the options yet since they are registered just a bit after.

elite-benni commented 1 month ago

Yeah it's a very tricky timing issue. If you want to debug the specific scenario when you have time you can comment-out lines 322-327 and check out the new story that I added. When setInitialSelectedOptions gets called inside writeValue, it won't be able to access the options yet since they are registered just a bit after.

Ok i see. So the problem is, that the initialvalue is not set correctly. PossibleOptionsChanged is called, when the Value is really available. In the past there has been problems with setting the possibleOptions when it was registered, because the input value was not set at this time, thats when possibleOptionsChanged was introduced. Your solution also seems to work without this problem, so the content children don't have this timing issue.

We now have to options:

use your solution with the options observable and remove the possibleOptionsChanged, because it is not needed anymore.

also save the initial value (string) in the brain service and also set the intialOptions on possibleOptionsChanged and drop the options$ in the component.


public setInitialSelectedOptions(value: unknown) {
    this.selectOptionByValue(value);
    this.state.update((state) => ({
        ...state,
        value: value as string | string[],
        initialValue: value as string | string[],
        initialSelectedOptions: this.selectedOptions(),
    }));
}
....
public possibleOptionsChanged() {
    this.selectOptionByValue(this.value());
    this.setInitialSelectedOptions(this.initialValue());
}

What I can think of a problem with your solution is, that when the value is written and the options are added afterwards without writing the value again, the select box would again react weirdly.

I hope I was able to explain it good enough. ;)

guillermoecharri commented 1 month ago

... We now have to options:

use your solution with the options observable and remove the possibleOptionsChanged, because it is not needed anymore.

also save the initial value (string) in the brain service and also set the intialOptions on possibleOptionsChanged and drop the options$ in the component. ... What I can think of a problem with your solution is, that when the value is written and the options are added afterwards without writing the value again, the select box would again react weirdly.

I hope I was able to explain it good enough. ;)

I updated the "ReactiveFormControlWithForAndInitialValueAndMultiple" story to play around with dynamically changing the available options and it seems to react appropriately for the most part.

The one case that is a bit strange is if an option you've selected gets removed. Maybe this is what you meant by "without writing the value again" since selecting another value would cause that to get cleared out.

It's a strange case but if the expected behavior is that the selected option gets de-selected then maybe this could be handled in the brain service when deregisterOption is called. Maybe something like:

public deregisterOption(option: CdkOption | null) {
    this._possibleOptions.update((options) => options.filter((o) => o !== option));
    // update value if it was just removed
    if (this.multiple()) {
        const values = this.value() as string[];
        const filteredValues = values.filter((v) => v !== option?.value);
        // only update if there was an actual change
        if (values.length !== filteredValues.length) {
            console.log('old values', values, 'new values', filteredValues);
            this.state.update((state) => ({
                ...state,
                value: filteredValues,
            }));
        }
    } else {
        const value = this.value() as string;
        if (value === option?.value) {
            this.state.update((state) => ({
                ...state,
                value: '',
            }));
        }
    }
}

From some testing it looks like this still isn't quite fixing the issue. Any thoughts on this approach?

edit: Oops I see that I probably got mixed up with state.value and state.selectedOptions. I'll look into this more later.

elite-benni commented 1 month ago

Yeah it seema like a little mess with "value" "selected options" and "initialvalue". I am not quite sure why this was needed. Maybe @thatsamsonkid has more insight to this.

guillermoecharri commented 1 month ago

I update the PR. The main changes are:

Now possibleOptionsChanged is no longer needed. I created an effect in the brn-select component that syncs the possibleOptions in the service when the options contentChildren query updates.
I was also able to get rid of initalSelectedOptions in favor of just using selectedOptions throughout.
I implemented the things that @elite-benni brought up about updating the selectedOptions if the possibleOptions are changed

I ran into the headache that @thatsamsonkid mentioned with the pristine/dirty state, but I was able to account for it by doing a 1-time markAsPristine when setting the initial value.

The only other thing of note was that I ran into some trouble getting brn-select-value to render the initial value properly. It seemed like using the signal it was taking an extra frame to render. The only work around I found for that was manually running change detection :( which is always a bummer.

thatsamsonkid commented 4 weeks ago

@guillermoecharri - nice work!, sorry I feel like I haven't been able to give this the attention it deserves. Been busy with personal matters. I will make sure to carve out some time to look at the latest changes later today. Honestly i'd be ok with the manual change detection check. I had run into a similar issue earlier in the development of select so honestly not the worst thing ever

guillermoecharri commented 4 weeks ago

Cleaned up according to the feedback :)

guillermoecharri commented 3 weeks ago

Updated to fix the minor comments

goetzrobin / spartan