-
Notifications
You must be signed in to change notification settings - Fork 32
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
fix: hide dropdown when custom messages are empty #220
Conversation
Thanks for the clear explanation and for going straight ahead with the fix. This is a good change! 👍 |
…gOptionsMsg or createOptionMsg would be shown but are falsy'
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.
Thank you for merging @janosh! I left a review on the console.error, lmk what you think.
if (allowUserOptions && !createOptionMsg) { | ||
console.error( | ||
`MultiSelect's allowUserOptions=${allowUserOptions} but createOptionMsg=${createOptionMsg} is falsy. ` + | ||
`This prevents the "Add option" <span> from showing up, resulting in a confusing user experience.` |
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'm a bit surprised by this change. I don't think it should error for this. It is used in the docs and is my use case. A user expects no message if it is set to an empty string.
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.
Ah, right. Hadn't thought about the tag component use case where you don't necessarily have a pre-defined set of tags.
Maybe we could treat createOptionMsg={null}
as a special case to signal you really don't want to show a createOptionMsg
so it shouldn't through an error. I still think it would be good to alert devevlopers if they pass createOptionMsg=""
or createOptionMsg={undefined}
since it might happen by accident.
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.
Yes, this sounds like a nice solution! I can open a new MR if you want :)
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.
That'd be great!
Closes #219
Summary of major changes
optionMessage
in a @const<li/>
element if the optionMessage is emptyadd_option_msg_is_active
tooption_msg_is_active
for clarityBefore
After
Checklist
[ ]
has tests for any new functionality or bug fixes-> Not applicable I believe[ ]
has examples/docs for any new functionality-> Not applicable