search frontend: allow multiple toggle state rules #16343
Conversation
| @@ -41,6 +41,7 @@ exports[`PlainQueryInput empty 1`] = ` | |||
| </div> | |||
| <div | |||
| aria-checked={true} | |||
| aria-disabled={false} | |||
There was a problem hiding this comment.
The previous value here had type boolean | undefined. Let's just use boolean and set it explicitly.
| if (rule.condition) { | ||
| disabled = true | ||
| tooltipValue = rule.reason | ||
| onToggle = (): void => undefined // Make the button untoggleable. |
There was a problem hiding this comment.
Question for @attfarhan: I want to make the button untoggleable when one of the conditions is true. I think this is always going to be the case. So: "if we should disable -> also make untoggleable", and that's what this change does. It works, but I'm not sure if there's a better way.
What I do know is that the current way isn't great: previous to this PR, we would duplicate the "is structural" check, once inside the onToggle callback, and again as a disabledCondition. When I added more conditions, I couldn't figure out why I could still toggle the button even though it was greyed out/disabled. Then I found we have another callback that duplicates the check.
This duplication gets complicated and the code structure doesn't work. We can avoid that now with this solution, and I'm happy that the loop/logic above will take care of that: any time a disabledOn rule is added, we guarantee it will not be toggleable. But, let me know if there's something I'm missing that could be bad about overwriting onToggle in this part of the code. So the question is: things are working, but is overwriting the onToggle here safe/reasonable or should I be watching out for something?
There was a problem hiding this comment.
Yeah this is fine, it should work. One thing you could do instead of reassigning onToggle is to make the onClick prop below take this: onClick={disabled ? undefined : onToggle} (this works since onClick is optional and can be undefined to denote no handler). IMO this makes it a bit clearer inline that the callback in onClick may change value based on disabled. Your call though, either way is fine.
There was a problem hiding this comment.
Nice, I like that! Cheers
There was a problem hiding this comment.
Alternatively, here's a proposal to convert <QueryInputToggle/> to a function component and make this logic more declarative thanks to hooks: #16351
There was a problem hiding this comment.
@lguychard I think I wanted something like that, and want that change. But I'm going to merge this stack to save me an approve+merge+rebase cycle :-) I'll see if I can rebase your branch after.
| if (patternType === SearchPatternType.structural) { | ||
| return | ||
| } |
There was a problem hiding this comment.
This disables the button from being toggleable, and previously the disabledCondition grayed it out. I found the logical separation of these two things confusing in the code, and don't think we should do this. When I add more things, I would have to inline more conditions/special cases into the other callbacks and it gets icky quickly. With the refactor, we don't need to check conditions inside the toggleable callback--instead, we will simply override the toggle callback to be the default (untoggleable).
Codecov Report
@@ Coverage Diff @@
## main #16343 +/- ##
=======================================
Coverage 52.92% 52.92%
=======================================
Files 1656 1656
Lines 82693 82698 +5
Branches 7455 7386 -69
=======================================
+ Hits 43764 43770 +6
+ Misses 35090 35088 -2
- Partials 3839 3840 +1
|
| if (rule.condition) { | ||
| disabled = true | ||
| tooltipValue = rule.reason | ||
| onToggle = (): void => undefined // Make the button untoggleable. |
There was a problem hiding this comment.
Yeah this is fine, it should work. One thing you could do instead of reassigning onToggle is to make the onClick prop below take this: onClick={disabled ? undefined : onToggle} (this works since onClick is optional and can be undefined to denote no handler). IMO this makes it a bit clearer inline that the callback in onClick may change value based on disabled. Your call though, either way is fine.
Helps with #13958 (comment). This refactor allows specifying multiple rules to disable toggles. See inline comments for more.