Skip to content

Feature/selector org #606

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.

Sign up for GitHub

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

Merged
interim17 merged 6 commits into from
Nov 18, 2024
Merged

Feature/selector org #606

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
667a0a5
add compoundSelectors to redux
interim17 Oct 31, 2024
25f4478
add ColorSettings enum and use container specific selector in ModelPanel
interim17 Oct 31, 2024
7ac652e
add currentColorSettings to selection branch state
interim17 Oct 31, 2024
9dae22e
rename ColorSetting from plural
interim17 Nov 8, 2024
1818d54
fix typo in selection branch spread in getCurrentUIData test
interim17 Nov 8, 2024
622b3b3
add explanatory comment to compoundSelectors
interim17 Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions src/containers/ModelPanel/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,7 @@ import {
requestTrajectory,
changeToNetworkedFile,
} from "../../state/trajectory/actions";
import {
getUiDisplayDataTree,
getIsNetworkedFile,
} from "../../state/trajectory/selectors";
import { getIsNetworkedFile } from "../../state/trajectory/selectors";
import {
AgentRenderingCheckboxMap,
ChangeAgentsRenderingStateAction,
Expand Down Expand Up @@ -44,6 +41,7 @@ import {
getSelectAllVisibilityMap,
getSelectNoneVisibilityMap,
getIsSharedCheckboxIndeterminate,
getUiDisplayDataTree,
} from "./selectors";

import styles from "./style.css";
Expand Down
75 changes: 69 additions & 6 deletions src/containers/ModelPanel/selectors.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import { initialState, State } from "../../state";
import {
getSelectAllVisibilityMap,
getSelectNoneVisibilityMap,
getIsSharedCheckboxIndeterminate,
getUiDisplayDataTree,
} from "./selectors";

const mockUiDisplayData = [
Expand Down Expand Up @@ -50,9 +52,8 @@ const mockUiDisplayData = [
describe("ModelPanel selectors", () => {
describe("getSelectAllVisibilityMap", () => {
it("Returns an agent visibility map with all possible states", () => {
const result = getSelectAllVisibilityMap.resultFunc(
mockUiDisplayData
);
const result =
getSelectAllVisibilityMap.resultFunc(mockUiDisplayData);
const expected = {
agentWithChildren1: ["", "state1"],
agentWithChildren2: ["", "state1"],
Expand All @@ -64,9 +65,8 @@ describe("ModelPanel selectors", () => {

describe("getSelectNoneVisibilityMap", () => {
it("Returns an agent visibility map with none of the possible states", () => {
const result = getSelectNoneVisibilityMap.resultFunc(
mockUiDisplayData
);
const result =
getSelectNoneVisibilityMap.resultFunc(mockUiDisplayData);
const expected = {
agentWithChildren1: [],
agentWithChildren2: [],
Expand Down Expand Up @@ -164,4 +164,67 @@ describe("ModelPanel selectors", () => {
expect(result).toBe(true);
});
});
describe("getUiDisplayDataTree", () => {
it("returns an empty array if ui display data is empty", () => {
expect(getUiDisplayDataTree(initialState)).toStrictEqual([]);
});
Comment on lines +167 to +170
Copy link
Contributor Author

@interim17 interim17 Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just moved

it("correctly maps agent display info to an array of display data", () => {
const state: State = {
...initialState,
trajectory: {
...initialState.trajectory,
defaultUIData: [
{
name: "agent1",
displayStates: [],
color: "#bbbbbb",
},
{
name: "agent2",
color: "#aaaaaa",
displayStates: [
{
name: "state1",
id: "state1_id",
color: "#000000",
},
{
name: "state2",
id: "state2_id",
color: "#000000",
},
],
},
],
},
};

const expected = [
{
title: "agent1",
key: "agent1",
children: [],
color: "#bbbbbb",
},
{
title: "agent2",
key: "agent2",
color: "#aaaaaa",
children: [
{
color: "#000000",
label: "state1",
value: "state1_id",
},
{
color: "#000000",
label: "state2",
value: "state2_id",
},
],
},
];
expect(getUiDisplayDataTree(state)).toStrictEqual(expected);
});
});
});
26 changes: 25 additions & 1 deletion src/containers/ModelPanel/selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,33 @@ import { createSelector } from "reselect";
import { isEmpty } from "lodash";

import { AgentDisplayNode } from "../../components/AgentTree";
import { getUiDisplayDataTree } from "../../state/trajectory/selectors";
import { getAgentVisibilityMap } from "../../state/selection/selectors";
import { AgentRenderingCheckboxMap } from "../../state/selection/types";
import { getCurrentUIData } from "../../state/compoundSelectors";
import { UIDisplayData } from "@aics/simularium-viewer";

export const getUiDisplayDataTree = createSelector(
[getCurrentUIData],
(uiDisplayData: UIDisplayData) => {
if (!uiDisplayData.length) {
return [];
}
return uiDisplayData.map((agent) => ({
title: agent.name,
key: agent.name,
color: agent.color,
children: agent.displayStates.length
? [
...agent.displayStates.map((state) => ({
label: state.name,
value: state.id,
color: state.color,
})),
]
: [],
}));
}
);

// Returns an agent visibility map that indicates all states should be visible
export const getSelectAllVisibilityMap = createSelector(
Expand Down
78 changes: 78 additions & 0 deletions src/state/compoundSelectors/compoundSelectors.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
import { getCurrentUIData } from ".";
import { initialState } from "..";
import { ColorSettings } from "../selection/types";

describe("getCurrentUIData", () => {
it("returns empty array if default UI data has not been entered yet", () => {
expect(getCurrentUIData(initialState)).toEqual([]);
});
it("returns selectedUIDisplayData if colorSetting is equal to ColorSettings.UserSelected", () => {
expect(
getCurrentUIData({
...initialState,
trajectory: {
...initialState.trajectory,
defaultUIData: [
{
name: "agent1",
displayStates: [],
color: "#bbbbbb",
},
],
},
selection: {
...initialState.selection,
currentColorSettings: ColorSettings.UserSelected,
selectedUIDisplayData: [
{
name: "agent1",
displayStates: [],
color: "#000",
},
],
},
})
).toEqual([
{
name: "agent1",
displayStates: [],
color: "#000",
},
]);
});

it("returns defaultUIData if colorSetting is euqal to ColorSettings.Default", () => {
expect(
getCurrentUIData({
...initialState,
trajectory: {
...initialState.trajectory,
defaultUIData: [
{
name: "agent1",
displayStates: [],
color: "#bbbbbb",
},
],
},
selection: {
...initialState.selection,
currentColorSettings: ColorSettings.Default,
selectedUIDisplayData: [
{
name: "agent1",
displayStates: [],
color: "#000",
},
],
},
})
).toEqual([
{
name: "agent1",
displayStates: [],
color: "#bbbbbb",
},
]);
});
});
31 changes: 31 additions & 0 deletions src/state/compoundSelectors/index.ts
View file
Open in desktop
Copy link
Contributor

@frasercl frasercl Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
import { createSelector } from "reselect";

import { getDefaultUIDisplayData } from "../trajectory/selectors";
import {
getCurrentColorSettings,
getSelectedUIDisplayData,
} from "../selection/selectors";
import { ColorSettings } from "../selection/types";
import { UIDisplayData } from "@aics/simularium-viewer";
Copy link
Contributor

@meganrm meganrm Nov 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: organize imports


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;
}
);
Copy link
Contributor Author

@interim17 interim17 Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important in forthcoming work but not functionally relevant until it replaces getSelectedUIDisplayData in getSelectionStateInfoForViewer

Mostly here to demonstrate the reasoning behind compoundSelectors

2 changes: 2 additions & 0 deletions src/state/selection/reducer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
SetRecentColorsAction,
SetSelectedAgentMetadataAction,
SetSelectedUIDisplayDataAction,
ColorSettings,
} from "./types";

export const initialState = {
Expand All @@ -36,6 +37,7 @@ export const initialState = {
recentColors: [],
selectedAgentMetadata: {},
selectedUIDisplayData: [],
currentColorSettings: ColorSettings.Default,
};

const actionToConfigMap: TypeToDescriptionMap = {
Expand Down
2 changes: 2 additions & 0 deletions src/state/selection/selectors/basic.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,5 @@ export const getSelectedAgentMetadata = (state: State) =>
state.selection.selectedAgentMetadata;
export const getSelectedUIDisplayData = (state: State) =>
state.selection.selectedUIDisplayData;
export const getCurrentColorSettings = (state: State) =>
state.selection.currentColorSettings;
Copy link
Contributor

@meganrm meganrm Nov 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@interim17 interim17 Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about currentColorSetting/getCurrentColorSetting for state and AgentColorMapping for the typing.

Examples:
if (colorSetting === AgentColorMapping.UserSelected)
or
currentColorSetting: AgentColorMapping.Default,

Copy link
Contributor

@meganrm meganrm Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

5 changes: 5 additions & 0 deletions src/state/selection/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,3 +87,8 @@ export interface SetSelectedUIDisplayDataAction {
payload: UIDisplayData;
type: string;
}

export enum ColorSettings {
Copy link
Contributor

@meganrm meganrm Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: make this singular. So when it's used: colorSetting.Default it makes sense. it's a single setting

Copy link
Contributor

@meganrm meganrm Nov 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

UserSelected = "userSelected",
Default = "default",
}
26 changes: 1 addition & 25 deletions src/state/trajectory/selectors/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
import { createSelector } from "reselect";
import { UIDisplayData } from "@aics/simularium-viewer";

import {
isNetworkSimFileInterface,
LocalSimFile,
NetworkedSimFile,
} from "../types";
import { getSimulariumFile, getDefaultUIDisplayData } from "./basic";
import { getSimulariumFile } from "./basic";

export const getIsNetworkedFile = createSelector(
[getSimulariumFile],
Expand All @@ -18,27 +17,4 @@ export const getIsNetworkedFile = createSelector(
}
);

export const getUiDisplayDataTree = createSelector(
[getDefaultUIDisplayData],
(uiDisplayData: UIDisplayData) => {
if (!uiDisplayData.length) {
return [];
}
return uiDisplayData.map((agent) => ({
title: agent.name,
key: agent.name,
color: agent.color,
children: agent.displayStates.length
? [
...agent.displayStates.map((state) => ({
label: state.name,
value: state.id,
color: state.color,
})),
]
: [],
}));
}
);

export * from "./basic";
Loading