search

mui / mui-x

MUI X: Build complex and data-rich applications using a growing list of advanced React components, like the Data Grid, Date and Time Pickers, Charts, and more!
https://mui.com/x/
3.79k stars 1.12k forks source link

[charts] Allow charts highlights to be controlled #12828

Open JCQuintas opened 2 weeks ago

JCQuintas commented 2 weeks ago

Currently the charts control the entire highlight model, with only some "configuration" coming out of the series[].highlightedScope property.

This proposal is to have highlight be controllable by users, so they can sync or control multiple highlights at the same time.

Improvements

mui-bot commented 2 weeks ago

Deploy preview: https://deploy-preview-12828--material-ui-x.netlify.app/

Generated by :no_entry_sign: dangerJS against 9a028face9b8034e0ebf39ecc0fb49aabea48d47

JCQuintas commented 2 weeks ago

@flaviendelangle thanks for the input, I didn't know about that hook :)

I'll probably remove the option to control the scope as that is a bit useless. Though this would still be useful for controlling the highlighted item data itself. 👍

alexfauquette commented 2 weeks ago

What we currently have

Those two contexts are updated at the same time with similar data.

The HighlightContext.item is never used. For now, all the components compute their highlighted/faded state based on the item of InteractionContext.

Why those two context

The HighlightContext exists because the selected series chooses who should fade. So this context allows us to provide this information to all the components without passing them every series.

The InteractionContext has a larger scope. It stores the current elements the mouse is interacting with. Which is used to highlight elements. But also to show/hide the tooltip. And show axis highlights.

Proposal

We could avoid the control of the control the scope by computing it from the selected item. If HighlightProvider has access to the series it will be possible from the selected series attributes type and id to get the highlight scope.

The components should use the HighlightContext.item to get their status. Otherwise, we will struggle to highlight the series without having the toolbar open.

The HighlightProvider could be simplified as follow


function HighlightProvider(props){
  const { series, highlightedItem, onHighlightedItemChange } = props

  const [item, setItem] = useControlled({..., controlled: highlightedItem, default: null})

  const scope = getScope(item, series)

  const enterItem = React.useCallback((newItem) => { 
    onHighlightedItemChange(newItem, { reason: 'enterItem'})
    setItem(newItem)
  } , [])

  const enterLeaveItem = React.useCallback((newItem) => { 
    // Always trigger the callback, but only update internal state if leaving the current state
    onHighlightedItemChange(newItem, { reason: 'leaveItem'})
    if (
        item !== null &&
        (Object.keys(newItem).every(
          (key) => newItem[key] === item[key],
        )
      ) {
        setItem(newItem)
      }
  } , [item])

  const value = React.useMemo(
    () => ({
      item,
      scope,
      enterItem,
      leaveItem,
      // Could add enterLegendSeries, leaveLegendSeries latter
    }),
    [data],
  );
}
JCQuintas commented 3 days ago

Ok, I think I understand where my mental model is wrong in all of this.

I seem to have an understanding that the dataset/series should be data related only, not display configuration. Kinda of like a table in a database, where dataset is the table, and series should be a column, you can then use sql to configure this data however you want.

dataset series visualisation
Database table column sql query
MUI-X dataset series series+other_props

What confuses me is that visualisation depends on series+ rather than only the component's props. I guess the above should look like:

dataset series visualisation
MUI-X dataset series props

This would allow us to move all visualisation props to the components' properties instead, making the dataset/series kinda of pure.

function BarsDataset() {
  return (
    <BarChart
      dataset={[
        { london: 59, paris: 57, seoul: 12, month: 'Jan' },
        { london: 50, paris: 52, seoul: 54, month: 'Fev' }
      ]}
      series={[
        { dataKey: 'london', label: 'London', id: 'series1' },
        { dataKey: 'paris', label: 'Paris', id: 'series2' },
        { dataKey: 'seoul', label: 'Seoul', id: 'series3' },
      ]}
      xAxis={[{ scaleType: 'band', dataKey: 'month' }]}
      yAxis={[{ label: 'rainfall (mm)' }]}
      highlight={...} // (1) see below
      layout={"horizontal" | "vertical"} // (2) see below
      stacks={{"stack1": ['series1', 'series2']}} // (3) see below
      stackOrder={...} // // (4) see below
    />
  );
}

This would allow us to streamline a bit the properties as well:

  1. Highlight doesn't seem to play well with different combinations between series. And moving it to "chart/plot" level would turn it more consistent.
  2. Having one series with horizontal config while another with vertical straight out breaks charts if both are to be rendered in the same place.
  3. Stacks can be made by series id instead.
  4. Stack order also doesn't seem to play well with different config by series.
  5. Probably other configs could be moved out, like color, similar to the stacks suggestion.

As a result, this would allow re-using dataset/series between multiple composed charts, since now series wouldn't be "type" constrained. This would probably make possible to have pie charts data follow the same structure as the other series, since it would be just data, we can configure it on a property of the pie chart/plot.

I understand this might be a bit shortsighted, as I don't have the context on why it was done the current way, also that this would take a considerable effort to achieve and that it should be left for a future BC, but I wanted to discuss it regardless 😅