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
#5580 - Add omitted field click handlers for the rest of the input types #5685
Conversation
Codecov Report
@@ Coverage Diff @@
## main #5685 +/- ##
=======================================
Coverage 64.65% 64.66%
=======================================
Files 1044 1044
Lines 32727 32735 +8
Branches 6203 6202 -1
=======================================
+ Hits 21161 21169 +8
Misses 11566 11566
|
@@ -112,6 +112,16 @@ const TemplateToggleWidget: React.VFC<TemplateToggleWidgetProps> = ({ | |||
onModeChange("number"); | |||
} else if (inputModeOptions.some((option) => option.value === "var")) { | |||
onModeChange("var"); | |||
} else if (inputModeOptions.some((option) => option.value === "select")) { |
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.
Does this need to be first before string and var? Won't all variables generally support a "var"? Or is this just meant to address the issues in the Mod Options setup?
} else if (inputModeOptions.some((option) => option.value === "select")) { | ||
onModeChange("select"); | ||
} else if ( | ||
inputModeOptions.some((option) => option.value === "boolean") |
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.
For boolean the behavior I'd like to see is toggling to boolean and setting the value to true. That's much more common than wanting to show the var input
You could also consider writing as a for-each-loop with a break
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.
@BLoe this is probably fine, but see comments about expected behavior
Also, this could be made much cleaner by creating a set of option values and then using includes/has instead of repeatedly using some
No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack. Do not edit this comment manually. |
What does this PR do?
Checklist