Feature/user colors #584
Feature/user colors #584
Conversation
… into feature/user-colors
interim17
requested review from
meganrm and
frasercl
and removed request for
a team
Coverage report
Show files with reduced coverage 🔻
Test suite run success121 tests passing in 7 suites. Report generated by 🧪jest coverage report action from c52e95d |
… into feature/user-colors
| const setInitialSelectedUIData = createLogic({ | ||
| process(deps: ReduxLogicDeps, dispatch) { | ||
| const { action } = deps; | ||
| const uiData = action.payload; | ||
| dispatch(setSelectedUIDisplayData(uiData)); | ||
| }, | ||
| type: SET_DEFAULT_UI_DATA, | ||
| }); |
There was a problem hiding this comment.
This logic looks like it just re-dispatches the triggering action with a different type? If not, what am I missing; if so, why not just use that action in place of the one that triggers this logic?
There was a problem hiding this comment.
Your point is valid!
Context: this was a larger PR, and I got the feedback that I should chunk things up in more bite size pieces. You're correct that this doesn't really need this logic yet, we can just call both setDefaultUIData and setSelectedUIDisplayData in ViewerPanel, and I would happily make that change for now.
I know because I have a crystal ball, that we are likely going to need this asynchronous logic because we won't know what happens first: parsing of the trajectory to derive defaultUIData and retrieval from browser storage of selectedUIData.
This is a problem of my making! Trying to work backwards from bigger feature branches into smaller PRs can lead to issues like this.
Maybe the defining of the subtasks is the real problem, as this whole PR is somewhat contrived and serves no purpose except to lay the groundwork for forthcoming code, so I wonder what is the greater sin: using this logic to set derived state, or correctly storing state that is never accessed in the first place.
| }); | ||
| dispatch(setDefaultUIData(newUiData)); | ||
| dispatch(setSelectedUIDisplayData(newUiData)); | ||
| done(); |
There was a problem hiding this comment.
Still a bit bummed to see this synchronous state derivation code in a logic. Everything this code does would (I think) work just fine in a reducer branch, and would probably be more at home there, where it wouldn't have to re-dispatch a new action and immediately call done to clear out resources held by redux-logic for what it assumes to be an async operation. That change is well out of scope for this PR though.
There was a problem hiding this comment.
This seems like a good opportunity for a refactor. This logic was put in place during previous work on color changing, and its role has changed.
We can derive the new UiDisplayData and pass it into redux directly, redux never needs to handle the ColorChange. I am inclined to do this in ModelPanel instead of in the reducer because the pattern in this repo is for reducers to simply pass on their action payload without side effects.
Adding (synchronous) logic to a reducer feels like breaking that pattern in a way that puts code in more places which is part of the pain of redux, for me at least. But I accept that this redux-logic is not the right place for it either.
There was a problem hiding this comment.
Wrote a brief issue (#596 ) and will address in a separate PR.
Time estimate or Size
small
Problem
Closes #579, advances #511
We need to differentiate between the default UI data derived by parsing trajectories and the current UI data that includes user color selections. This PR adds
selectedUIDisplayDatato redux, and points thegetSelectionStateInfoForViewerselector at it. When default arrives it sets the initial state for selected, and then as users make color changes we only update the selected data, and the default remains unchanged.Steps to Verify: