-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: ๐ธ Add SQFormMultiValue component #306
Conversation
Listening to your loom, what happens if I type in a string that is equal to the Example: options = { red: 1, blue: 2} and I type in 2. |
Good question. So for your example, nothing. BEcause 2 and "2" are different. But if you had { red: 'color-red', blue: 'color-blue' } and you typed in "color-blue" you'll end up with just "Blue" in the options seen here: https://www.loom.com/share/8f331a81f57c4bf5bb533cbc0c03d126 |
What happens if your selected options take up more width than the field? |
stories/SQForm.stories.js
Outdated
<SQFormMultiValue | ||
name="favoriteColors" | ||
label="Your Favorite Colors" | ||
size={12} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Storybook is great for what I call UI chaos testing.
I question what happens if we change this to size 6
then select all of the options. What happens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@20BBrown14 what is that little x all by itself on the right there? I didn't see that in the Loom. Does that clear all the fields including initial values? Make sure this gets ran by Chris as well since it wasn't part of the original Loom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pointed that out in the loom as a way to clear the field at about 43 seconds
edit: and is also how SQFormAutocomplete works so our users and devs should be familiar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pointed that out in the loom as a way to clear the field at about 43 seconds
edit: and is also how SQFormAutocomplete works so our users and devs should be familiar.
Yep. It's a great feature to have! I think you'll want to run the clear button by Nemeth. My hunch is he's going to want it to always stick to the bottom regardless of the number of rows so it's always in the same spot near the inputs bottom border. The default from MUI sort of looks likes its floating in no mans land.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert react-scripts downgrade, update story
Made it so the little x button that clears the input is always near the bottom of the input
We are missing component documentation for this component. Adding the validation example and reasoning would be good in the documentation. |
I'm not sure what you're referring. Is there some documentation for the components I wish i knew about several months ago? |
We do notes on some components. We used to do it a lot more. I am not sure where the stance is but it is nice to have for things that need a little more explanation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic work @20BBrown14!
Great job building this component @20BBrown14 , accepting PR feedback, and turning the feedback into a polished solution ๐ฏ I stamped my approval, pending the removal of the package-lock.json that's still there. |
Ah, that's fair. I added a notes page that explains some stuff. Validation would probably be the trickiest thing. I also added component stories, which I left out originally. |
Thanks for all the feedback and approvals. Merging since everything seems to be addressed ๐ ๐ฎ |
Add new component, SQFormMultiValue
Loom: https://www.loom.com/share/e2405d5fbe1a42b7a7d0b457132c5593
Sorry for the loom length :/ the last half is just code, though, if you care about that.
โ Closes: #303
When this is approved I'll add an issue to add tests for this componentPost-merge edit: As promised, here is the issue for tests #307