feat: add some better value selection when providing multiple keys #110
Conversation
- Return with a value from the top level if it's not set in the keys[] - Return with a default if it's not set in the keys[] - Do not render a dropdown if the value hasn't come from the keys[] - Do not render a dropdown if there's only one value in the keys[]
gratcliff
left a comment
gratcliff
left a comment
There was a problem hiding this comment.
That's quite the change! Code looks solid to me.
| shouldShowVarDropdown(selected) { | ||
| const { user, variable } = this.props; | ||
|
|
||
| return !!this.getSelectedValue(selected)[variable] && Array.isArray(user.keys) && user.keys.length > 1; |
There was a problem hiding this comment.
Should it only count keys with values? Is that possible?
| return !!this.getSelectedValue(selected)[variable] && Array.isArray(user.keys) && user.keys.length > 1; | |
| return !!this.getSelectedValue(selected)[variable] && Array.isArray(user.keys) && user.keys.filter(key => key.name === variable).length > 1; |
There was a problem hiding this comment.
We're already checking if this particular variable has a value assigned in the first part of this statement:
!!this.getSelectedValue(selected)[variable]Is that sufficient for the use case you're referring to? If there isn't a user.keys[variable] then it'll already have exited early from that statement because we already know not to show the var dropdown. So at that point, checking the length should suffice? Unless i'm misunderstanding.
There was a problem hiding this comment.
Gotcha gotcha, misread that.
|
lgtm, @domharrington want me to merge and tag a release for you? |
|
@erunion yes please 🙏 |
🧰 Changes
🧬 QA & Testing
Do the tests pass? ✅
I've tested this npm linked into ReadMe but I wanna do a little more full QA to ensure all of these different edge cases are working as expected over there.