VEuPathDB / web-monorepo

A monorepo that contains all frontend code for VEuPathDB websites
Apache License 2.0
2 stars 0 forks source link

Map - either use `isVisible` prop or get rid of it #1008

Open moontrip opened 6 months ago

moontrip commented 6 months ago

Separated from https://github.com/VEuPathDB/web-monorepo/issues/935 as a tech debt

isVisible tech debt

At the moment, both visualizationPanelConfig and studyDetailsPanelConfig (in appState.ts) are of type PanelConfig which is

const PanelConfig = t.type({
  isVisible: t.boolean,
  position: PanelPositionConfig,
  dimensions: t.type({
    height: t.union([t.number, t.string]),
    width: t.union([t.number, t.string]),
  }),
});

The SubStudies (list of sub-studies) floater is conditionally rendered based on appState.studyDetailsPanelConfig?.isVisible but the DraggableVisualization (supporting plot) is rendered based on the truthiness of various variables, ultimately dependent on the truthiness of its activeVisualizationId prop.

There is currently logic setting/unsetting visualizationPanelConfig.isVisible but it is not used to conditionally render the panel.

We could either a) remove isVisible as a prop from visualizationPanelConfig and remove the logic setting/unsetting isVisible or b) standardise the behaviour and make DraggableVisualization conditional rendering make use of visualizationPanelConfig.isVisible

Given that the showControls work could involve changing the props of visualizationPanelConfig - maybe a) is the better option?

Bob said he preferred to have outer conditional rendering so this ticket will be set to remove internal conditional rendering. Note that my initial attempt revealed that there existed many type issues if removing internal condition.

dmfalke commented 6 months ago

My 2 cents: I think we should keep the isVisible property and get rid of activeVisualizationId. There have been discussions about allowing multiple floating visualizations, and this will put us in a better position to deliver that.