Share the All exclusivity rule between MultiValueVariable and the picker - #1601
Draft
samjewell wants to merge 1 commit into
Draft
Conversation
The rule was implemented twice: changeValueTo decided by array position (the last value wins) while the picker's enforceAllExclusivity decided by the react-select action. That is the same duplication that let the dropdown and the commit path drift apart in the first place, and the two copies would disagree for any selection not driven by the picker. react-select appends a newly selected option to the end of the value array, so the position rule already covers every case the action rule handled. Extract it as applyAllValueExclusivity and call it from both, which also stops changeValueTo mutating the value and text arrays it is handed. Co-authored-by: Sam Jewell <samjewell@users.noreply.github.com>
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #1596 — this targets
sj/query-var-all-exclusivity, so the diff here is only the refactor. Opened for comparison; fold it into #1596 or drop it.What
#1596 implements the All exclusivity rule twice:
changeValueTodecides by array position — if the last value is the All value it wins, otherwise the All value is dropped from a mixed selection.enforceAllExclusivityinVariableValueSelectdecides by react-select action — if the user selected the All option it wins, otherwise the All value is filtered out of a selection with more than one entry.This extracts a single
applyAllValueExclusivityhelper and calls it from both.Why
The duplication is the same shape as the bug #1596 fixes: the picker and the commit path each carrying their own copy of the rule is exactly how they drifted apart. Two copies also means two behaviours — they agree for every picker-driven path, but only the model's copy applies to programmatic updates, URL values and anything else that reaches
changeValueTodirectly.The position rule subsumes the action rule because react-select appends a newly selected option to the end of the value array, so "the user just selected All" and "the All value is last" are the same statement for the picker. That is why every existing test, including the three interaction tests and the toggle-all header test added in #1596, passes unchanged.
Side effect worth noting: the helper copies rather than splices in place, so
changeValueTono longer mutates thevalueandtextarrays its caller handed it.The helper is exported from the module but deliberately not added to the package index — it stays internal until something outside needs it.
Testing
applyAllValueExclusivity: the All value added last, mid-array and first; selections without the All value; missing and non-array text; and that it does not mutate its inputs.scenes-app"Variables with object values" demo) that the picker behaviour is byte-for-byte the same as VariableValueSelect: Make All value selection exclusive in the multi picker #1596: ticking a value with All selected deselects All and commits the concrete value; ticking All then another value commits only that value, with no$__allreaching state.