Skip to content
This repository has been archived by the owner on Sep 26, 2022. It is now read-only.

fix(ItemGroup): make value an array if it's not an index #94

Merged
merged 1 commit into from
Mar 4, 2021

Conversation

DinCahill
Copy link
Contributor

@DinCahill DinCahill commented Jan 3, 2021

Hi, I've tried to fix #90 and have something that seems to work. Hopefully someone can use this to fix it properly.

This fixes multiple broken playgrounds and examples in the docs.

value = [val] fixes dropdowns like these. Then I had to add typeof (val) === 'number' to stop the first three of these ListItemGroups from breaking.

Preview: https://svelte-materialify-git-fork-dincahill-fix-itemgroup.thecomputerm.vercel.app/components/alerts/#playground
Preview: https://svelte-materialify-git-fork-dincahill-fix-itemgroup.thecomputerm.vercel.app/components/list-item-groups/

@vercel
Copy link

vercel bot commented Jan 3, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/thecomputerm/svelte-materialify/mvpgk90r7
✅ Preview: https://svelte-materialify-git-fork-dincahill-fix-itemgroup.thecomputerm.vercel.app

@Florian-Schoenherr
Copy link
Collaborator

Thank you! This has to do with the implementation of multiple, I think.

@Florian-Schoenherr Florian-Schoenherr marked this pull request as ready for review February 2, 2021 16:29
@Florian-Schoenherr Florian-Schoenherr merged commit 9fe0fa3 into TheComputerM:master Mar 4, 2021
@kickermeister
Copy link
Contributor

This change broke my application. I'm using Select components without the multiple prop and I'm receiving now the values (which are strings and no numbers) within an array when binding to the value prop. I'm afraid there will be more users affected by this change... So I wondered whether a workaround could be found which doesn't change the original behavior (of course only if that was not the intention)?

@Florian-Schoenherr
Copy link
Collaborator

Florian-Schoenherr commented Mar 10, 2021

@kickermeister First of all, I'm really sorry.
We would need to at least add one more example to the Select docs which covers your case.
Afterwards, we can turn that statement around, I hope?

old:
} else value = val;

wrong:

 } else if (typeof (val) === 'number') {
        value = val;
      } else value = [val];

might work:

 } else if (typeof (val) === '???') {
        value = [val];
      } else value = val;

(where ??? is replaced by whatever the select in alerts/#playground gives back. The docs code for this also looks kinda sus, maybe that's the problem.)

The Select had some changes for just fixing up things, we want to rebuild it eventually.

@kickermeister
Copy link
Contributor

No need to worry in my case, the application is still under development and not yet published. 😊
I'm not sure that a new example is needed, since the basic one covers more or less my use case (except for the value binding). I'm also using strings for both, names and values of the items. So actually not very exotic?

@Florian-Schoenherr
Copy link
Collaborator

@kickermeister on the basic one for example you can also see a visual bug, the selected value is not primary color.
The docs should then just add a value binding and show what it gives back (easy visual test, until we have better tests).

Florian-Schoenherr added a commit that referenced this pull request Mar 17, 2021
Signed-off-by: Florian-Schoenherr <florian.schoenherr99@gmail.com>
@Florian-Schoenherr
Copy link
Collaborator

@kickermeister try the new release 👍

@kickermeister
Copy link
Contributor

Looking good - thanks a lot for the quick fix!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The Playground in Alert Component docs has some bugs
3 participants