Feature/selector org #606
Feature/selector org #606
Conversation
| describe("getUiDisplayDataTree", () => { | ||
| it("returns an empty array if ui display data is empty", () => { | ||
| expect(getUiDisplayDataTree(initialState)).toStrictEqual([]); | ||
| }); |
Coverage report
Show new covered files 🐣
Test suite run success132 tests passing in 8 suites. Report generated by 🧪jest coverage report action from 622b3b3 |
| export const getCurrentUIData = createSelector( | ||
| [ | ||
| getCurrentColorSettings, | ||
| getSelectedUIDisplayData, | ||
| getDefaultUIDisplayData, | ||
| ], | ||
| ( | ||
| colorSetting: ColorSettings, | ||
| sessionData: UIDisplayData, | ||
| defaultData: UIDisplayData | ||
| ) => { | ||
| const fileHasBeenParsed = defaultData.length > 0; | ||
| if (!fileHasBeenParsed) { | ||
| return []; | ||
| } | ||
| if (colorSetting === ColorSettings.UserSelected) { | ||
| return sessionData; | ||
| } | ||
| return defaultData; | ||
| } | ||
| ); |
There was a problem hiding this comment.
Important in forthcoming work but not functionally relevant until it replaces getSelectedUIDisplayData in getSelectionStateInfoForViewer
Mostly here to demonstrate the reasoning behind compoundSelectors
interim17
requested review from
ShrimpCryptid,
frasercl and
meganrm
and removed request for
a team and
ShrimpCryptid
src/state/compoundSelectors/index.ts
Outdated
| getSelectedUIDisplayData, | ||
| } from "../selection/selectors"; | ||
| import { ColorSettings } from "../selection/types"; | ||
| import { UIDisplayData } from "@aics/simularium-viewer"; |
src/state/selection/types.ts
Outdated
| type: string; | ||
| } | ||
|
|
||
| export enum ColorSettings { |
There was a problem hiding this comment.
nit: make this singular. So when it's used: colorSetting.Default it makes sense. it's a single setting
There was a problem hiding this comment.
I don't have a good suggestion for this name, but it could be confusing because at first glance it seems like it's going to be a list of colors. the singular will help, but something like, colorsToUse or colorsOrigin?
| export const getSelectedUIDisplayData = (state: State) => | ||
| state.selection.selectedUIDisplayData; | ||
| export const getCurrentColorSettings = (state: State) => | ||
| state.selection.currentColorSettings; |
There was a problem hiding this comment.
should also be singular (otherwise it sounds like it's going to be a list of colors, when it's just the singular binary setting)
There was a problem hiding this comment.
What about currentColorSetting/getCurrentColorSetting for state and AgentColorMapping for the typing.
Examples:
if (colorSetting === AgentColorMapping.UserSelected)
or
currentColorSetting: AgentColorMapping.Default,
There was a problem hiding this comment.
I wouldn't call it a Mapping because that generally means it's a lookup "table". but this is just the value you use in another lookup
There was a problem hiding this comment.
Maybe a short comment at the top of this file that describes what this file is for, like you did in the PR description?
I also see that the parent state directory has a README.md that (among other things) goes over the file structure for all this redux stuff - consider updating that as well
Time estimate or Size
small/medium
Changes related to directory organization, connected to #607, will not merge to main until both PRs are approved
Problem
Advances #511
Solution
@frasercl raised some interesting concerns about state branches in a previous PR
We have redux selectors in
/statethat are:ModelPanel,ViewerPanel, etc.)Now adding
compoundSelectorsto/stateto account for selectors that consume state from multiple branches and are used multiple containers.This resolution keeps the actual data for
defaultUIDisplayDatain thetrajectorybranch andselectedUIDisplayDataandColorSettingsin theselectionbranch, and avoids circular dependencies when the branches try to import each other's selectors.This work is essentially part of #607, but so many lines of moving/organizing code was obscuring the readability of that PR. I will not merge either PR until both are approved. It is a non-breaking change, the app should run as before.
getCurrentUIData
The selector which pulls from multiple branches to determine the current color settings for the app, and to pass to the viewer.
getUiDisplayDataTree
Previously in
trajectorymoved toModelPanelas it depends on a compound selector, and is only used in once place, could also be incompoundSelectorsfolder...