bugfix/inputchips-updates-twice

[Mahmoud-zino]

Linked Issue

Closes #1759

Description

after deleting

$: chipValues =
		value?.map((val, i) => {
			if (chipValues[i]?.val === val) return chipValues[i];
			return { id: Math.random(), val: val };
		}) || [];

every thing works as expected and the value is updated only once.

I have tested it with all use cases from the docs.

Changsets

bugfix: InputChips updates bound value only once.

Checklist

Please read and apply all contribution requirements.

• This PR targets the dev branch (NEVER master)
• Documentation reflects all relevant changes
• Branch is prefixed with: docs/, feat/, chore/, bugfix/
• Ensure Svelte and Typescript linting is current - run pnpm check
• Ensure Prettier linting is current - run pnpm format
• All test cases are passing - run pnpm test
• Includes a changeset (if relevant; see above)

[changeset-bot]

🦋 Changeset detected

Latest commit: fb583a2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@skeletonlabs/skeleton	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
skeleton-docs	✅ Ready (Inspect)	Visit Preview	Jul 18, 2023 6:32pm

[endigo9740]

@Mahmoud-zino checking the Git Blame I see @Sarenor introduced this portion of the code per:

• cbef3c3
• Added nullsafe access and nullish coalescense to InputChips to handle value being undefined. #1678

It looks like this was added to allow for null values to be handled by the autocomplete. Can the two of you sync on this and determine the best course of action here? My gut is telling me that just flat out removing this would introduce a regression.

[Sarenor]

I just added the null-check, the initial code was added to handle duplicate values:
2ba426b
(by Chris, at that ;) ) which seems to work though, so the ID-Mapping is no longer necessary? Seems kind of strange but I'll take it!

Passing in an undefined list breaks the component though, and I'm not quite sure if we want that as intentional behaviour?

[endigo9740]

Hmm, I wonder why the blame showed you. That's odd.

[Sarenor]

Cause it was me that changed those lines the last time when I added the || []; and prettier reformatted the lines :)

[Mahmoud-zino]

I will get back to this issue once I am done with the tree-view, I want to handle undefined without a reactive assignment, once this is done I think we can safely delete the reactive assignment.

[Mahmoud-zino]

@Sarenor

Passing in an undefined list breaks the component though, and I'm not quite sure if we want that as intentional behaviour?

can you please provide your setup, I tried passing undefined and got no errors, It is working as expected, maybe I am missing something here...

[endigo9740]

@Sarenor just pinging you to remind you about @Mahmoud-zino request above.

[Sarenor]

@Mahmoud-zino Sorry, totally missed this while I was camping.
Got the reproduction here:
https://github.com/skeletonlabs/skeleton/compare/dev...Sarenor:skeleton:undefined-input-chip?expand=1
If I set the anything-list in the docs to undefined the docs page for InputChip doesn't load anymore

[Mahmoud-zino]

@Sarenor this happens because we are accessing the lists to display what is selected/allowed/not-allowed etc... on the docs page. but the error is not coming from the component.

so commenting this:

<code class="code">{anythingList.length ? anythingList.join(', ') : 'No chips available.'}</code>

the component will work as expected.
If you can confirm, I will mark the PR as ready for review

[Sarenor]

@Mahmoud-zino you are indeed correct and I'm a fool!

[Mahmoud-zino]

@Sarenor no no, it is just easy to miss 👍

[endigo9740]

Thanks for your cooperation @Mahmoud-zino @Sarenor

I think with that we're good to move forward with this. I'll merge and we'll monitor the reaction from end users going forward.