mui-x icon indicating copy to clipboard operation
mui-x copied to clipboard

[charts] Allow charts highlights to be controlled

Open JCQuintas opened this issue 1 year ago • 4 comments
trafficstars

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.

  • Right now this is not really working
  • It doesn't take the series configuration into account

Improvements

  • Possibly offer a function to define which item is highlighted instead of the highlighted/focused properties

JCQuintas avatar Apr 18 '24 09:04 JCQuintas

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

Updated pages:

Generated by :no_entry_sign: dangerJS against 5cf98c7b92d4e5b1fb19a9b77221796a158ab14b

mui-bot avatar Apr 18 '24 09:04 mui-bot

@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. 👍

JCQuintas avatar Apr 18 '24 11:04 JCQuintas

What we currently have

  • HighlightContext which contains
    • item
    • scope
  • InteractionContext which contains
    • item
    • axis.x
    • axis.y

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],
  );
}

alexfauquette avatar Apr 18 '24 13:04 alexfauquette

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 😅

JCQuintas avatar Apr 30 '24 14:04 JCQuintas

@Janpot Does it match your need or did we miss something?

👍 Looks like it should do the trick. Thanks!

Janpot avatar May 28 '24 10:05 Janpot

https://github.com/mui/mui-x/assets/2109932/714cd454-8b25-4bd8-be6e-e82de5b683be

Trying it out a bit more, In a line chart I seem to only get information when I actually hover over a single point, even though the highlight is active.

And the other way around, I'd like to be able to highlight a whole row in a datagrid and highlight a corresponding vertical line for an axis, across all series (each representing a column)

Janpot avatar May 30 '24 13:05 Janpot

Screen.Recording.2024-05-30.at.15.51.04.mov Trying it out a bit more, In a line chart I seem to only get information when I actually hover over a single point, even though the highlight is active.

Yeah that is a current limitation of the line charts. I believe @alexfauquette mentioned we wanted to add a Voronoi handler for it to behave more like the Scatter charts, where the point closest to the mouse is highlighted/hovered.

Related to this issue https://github.com/mui/mui-x/issues/12840

JCQuintas avatar May 30 '24 14:05 JCQuintas

👍 Thanks, I see, that makes sense. But even then I think I'd like to know whether the cursor is on a specific point, or whether it's "close to"

Janpot avatar May 30 '24 14:05 Janpot

If the line appears to be highlighted independently to the controlled stated, it is because we only support item highlight in the HighlightProvider, and what you see is an axis highlight.

For now we use the InteractionProvider to render axis highlight.

It's something we will have to standardize, but for now it's still unclear how we should store all those information

https://github.com/mui/mui-x/blob/master/packages/x-charts/src/LineChart/LineHighlightPlot.tsx/#L47-L52

alexfauquette avatar May 30 '24 15:05 alexfauquette