Skip to content
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.

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

BaseVisualization and Top-Panel refactor #773

Closed
5 tasks done
adrianmroz-allegro opened this issue Jun 16, 2021 · 2 comments
Closed
5 tasks done

BaseVisualization and Top-Panel refactor #773

adrianmroz-allegro opened this issue Jun 16, 2021 · 2 comments

Comments

@adrianmroz-allegro
Copy link
Contributor

adrianmroz-allegro commented Jun 16, 2021

Issue for tracking technical decisions and progress.

To accomplish #756 and connected issues, we need first to pull top-panel into visualisation itself. To do this, we need to clean up code around BaseVisualization.

  1. First step is to remove BaseVisualization as a base class. Composition, not encapsulation
  • Remove scroll state from BaseVisualization. Different visualisations use scroll differently and they should be responsible to track this. For example Table is not interested in horizontal scroll at all. Move scroll state to specific visualisations #775
  • Remove dragOnSeries from state. It is not used. Remove old and unused code #776
  • Create component for managing highlight. It would keep Highlight state and provide useful callbacks/getters HighlightController #777
  • Extract data fetching into separate component. Visualisation can provide query function and/or shouldRefetchData predicate. Component will render error and loading states and provide to visualisation data as props iff in loaded state. DataProvider #779
  1. We need to clean up components in top-panel.
  • Remove references to Components. All communication should be done through props and/or state. "Dirty" tiles should be added to Essence. It is ephemeral state, not persisted into url/view definition and can be reset when reconstructing Essence Partial Tiles #780
  1. All generic solution will be encapsulated in CenterPanel component with following props:
  • visualisation - Component for rendering chart. Get's few props like data, highlight etc.
  • topMenu - Component for rendering top menu - splits, filters etc.
  • shouldFetchData - predicate for deciding if props change constitute data refetch. Optional.
  • makeQuery - function that creates Plywood query from Essence. Optional.
  1. All Visualizations will use CenterPanel component and provide props. For LineChart I imagine something like this:
function LineChartVisualization() {
  return <CenterPanel
   visualisation={LineChart}
   topMenu={LineChartTopMenu} 
  shouldFetchData={Essence.equals}
  />;
}

Because JSX invocation can also take render prop, we are free to customise existing components even if signature does not fit, like visualisation={({data, highlight}) => <LineChart series={data[0]} hasHighlight={highlight.hasHighlight()} changeHighlight={highlight.setNewHighlight} />

@adrianmroz-allegro
Copy link
Contributor Author

  • "Dirty" tiles should be added to Essence. It is ephemeral state, not persisted into url/view definition and can be reset when reconstructing Essence

It should be moved to separate provider. Let's not fill Essence with ephemeral state.
Biggest cons to this was: If Essence keep placeholders it should:
a) keep them separate, which is cumbersome - we spill state around.
b) keep them inline with regular filters/splits/series, but that way we would need to check all getters if they should return full tiles with or without partials. And I mean all of them. Like filteredOn should return true if we have partial filter on specific dimension?

There was of course something in between: put placeholders inside filters and series objects, beside main collections. I just didn't fancy that solution, but maybe in the future it will be better. Keep in mind.

@adrianmroz-allegro
Copy link
Contributor Author

  • shouldFetchData - predicate for deciding if props change constitute data refetch. Optional.

It looks like all visualisations use the same check here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant