What do you mean with

If the dataIndex is provided

provided by whom?

Maybe we want to rewrite most text instead

doesnt seem to add much from the phrase above it

In LineItemIdentifier we use dataIndex?: number. Should we decide on whether to use dataIndex?: number or dataIndex: number | null? It might be confusing otherwise.

I guess this is what "get priority" means, right?

Why do we need to prioritize data index over value? Can't we just use one or the other?

Should we call the first argument pointerIds instead of newAxes? They aren't axes, they are pointer identifiers.

Not sure if spreading the whole store will impact performance negatively, especially since it's being called on pointer move.

And potentially twice if onXAxisInteraction and onYAxisInteraction are both set.

What if we move the second argument of selectorChartsInteractionXAxisIndex and selectorChartsInteractionXAxisValue into standalone functions and then we provide them to the selector while also calling them here?

export function selectChartsInteractionXAxisValue(x: number | null, xAxes: ComputeResult<ChartsXAxisProps>, xIndex: number | null, id: AxisId | undefined) {
    if (x === null || xIndex === null || xAxes.axisIds.length === 0) {
      return null;
    }
    return valueGetter(x, xAxes, xIndex, id);
  },
}

export const selectorChartsInteractionXAxisValue = createSelector(
  [
    selectorChartsInteractionPointerX,
    selectorChartXAxis,
    selectorChartsInteractionXAxisIndex,
    optionalGetAxisId,
  ],
  selectChartsInteractionXAxisValue
);

And then in this file, we would do:

          const nextXAxisValue = selectChartsInteractionXAxisValue(svgPoint.x, xAxes, nextXAxisIndex, axisId);

The same would apply for the Y axis and for the X axis index.

What do you think?

Do we need the exclamation mark? Spreading undefined or null into an object is fine:

const a = { ...undefined }
// a = { } 

Yes, but in that case the value of interaction: { ...store.value.interaction, pointer: svgPoint }, could be interaction: { pointer: {x, y} } (if the interaction is undefined) which is not a valid interaction value. So TS complains in the selectors

Do we need the direction? The direction can be derived from the list of axes and the axis ID, right? So isn't the direction a bit redundant here?

It would allow us to have just one AxisItemIdentifier instead of having to split by cartesian/polar.

With the current implementation, it would be hard, because

const axis = useXAxis(id) ?? useYAxis(id)

On the principle, I agree. The current axis management could be improved to remove this x/y distinction in the storage. But that would be a much larger PR.

But that would be a much larger PR.

Wouldn't it be exporting a useCartesianAxes and a useCartesianAxis hooks whose implementation is basically return useXAxis(id) ?? useYAxis(id)? Or am I missing something?

It would save us from exposing the direction here.

Is there ever a case for a chart with cartesian and polar axes simultaneously? If so, we might have to expose a useAxes because a user doesn't know the axis type (cartesian or polar) to use the proper hook.

Is there ever a case for a chart with cartesian and polar axes simultaneously?

No there is no case like that.

We could export useCartesianAxis(id) but we still need useXAxis() and useYAxis() because they default to first axis in each direction

we still need useXAxis() and useYAxis() because they default to first axis in each direction

Sure, we can keep it, although the difference in behavior should only be theoretical. I believe we issue a warning when an x-axis has the same ID as a y-axis, so in practice the behavior should be the same as users aren't using the same IDs for different axes.

Should we wrap this in an useEventCallback?

If onAxisInteraction is a function that's created on every render, we might get into infinite loops because we run this useEffect whenever onAxisInteraction changes, which might call onAxisInteraction with newAxisPointerInteraction which is a new array every time. If onAxisInteraction causes a re-render (e.g., by calling setState), then we'll get an infinite loop.

We could, but I'm not a big fan of memorizing user functions.

About the inifit loop, I don't think it's an issue since onAxisInteraction is only called inside pointer events callback

I'm not a big fan of memorizing user functions

Why not?

I don't think it's an issue since onAxisInteraction is only called inside pointer events callback

You're right. It might just be inefficient to be removing and adding event listeners on every re-render. Not sure if we want to address that now.

when pointer moves from one axis value to another

Is this the only case? Can't it be called if the data changes as well?

I think something like "when the axis value changes" would work better.

Also, we use "axis value" here, but "axis item" below. Are they the same thing? Should we use the same name in both places?

Is this the only case? Can't it be called if the data changes as well?

It's the case for performance reasons. Otherwise each pointer move will trigger it.

If user what to have value updates, they should get full control on their events with https://mui.com/x/react-charts/components/#scales

Otherwise each pointer move will trigger it

Sorry, didn't understand why every pointer move would trigger the callback. Why do you say that's the case?

I misunderstood your initial comment.

I was focusing on the fact I remove the value to only keep the dataIndex attribute.

Can't it be called if the data changes as well?

I would be great but not implemented in this PR. I will have a look if the new selector implementation makes it easier to do

I misunderstood your initial comment.

Sure, no problem.

I would be great but not implemented in this PR. I will have a look if the new selector implementation makes it easier to do

I see. Should we still update the docs so that it's clear that it isn't only pointer move that will cause this event handler to be called?

Also, we use "axis value" here, but "axis item" below. Are they the same thing? Should we use the same name in both places?

I think we also missed the question above

Does this contain all axis items or only the ones that changed? We should probably specify that

Only those for axis with data property.

Yeah, but my point is: does newAxisInteraction contain only values that changed or the new interaction state?

For example, if there's an x-axis and an y-axis and I move the pointer horizontally such that only the x-axis value changes, do I get an array with one or two values?

It contains all the axes with a data property.

If one axis have a new dataIndex we return all the axes data index with their current highlight index. Such that the array always have the same length/order

Such that the array always have the same length/order

Thanks, that answers my question! Should it be part of the docs? Otherwise I suspect users might have the same doubts I had

So, if no axis highlight exists, the argument is an empty array, right?

One per axis.

How does this work? I'm trying the example below and there's an x and a y axes, but onAxisInteraction only returns:

[
    {
        "axisId": "x-axis",
        "dataIndex": 4
    }
]

Which axes are supposed to be shown?

Isn't this the same as the content on line 60?

Isn't it a bit weird that the prop that sets the highlighted is called highlightedAxis, but when it changes, the event handler is called onAxisInteraction?

Should they be axisInteraction/onAxisInteraction or highlightedAxis/onAxisHighlight?

My concern is the upcoming tooltip control that will need the same callback to know which axis needs to be displayed.

But we could effectively expose an onAxisHighlight and trigger another function for the tooltip.

This just open a new question. Should onAxisHighlight get an array of axis item or just the first one.

My concern is the upcoming tooltip control that will need the same callback to know which axis needs to be displayed.

Sorry, what do you mean?

But we could effectively expose an onAxisHighlight and trigger another function for the tooltip.

Ah, do you mean onAxisHighlight for axis highlights and onTooltipChange when the tooltip should display different items?

Should onAxisHighlight get an array of axis item or just the first one.

I think that's what I'm trying to get at in my comment below. I think there might be more than one axis highlighted at a time, so we probably need an array.

Can't we have more than one highlighted axis item?

In theory it's not that hard. In practice I don't see usecases.

I can think about the crosshair but when following the point, I feel doing store update would be a bad trade off in terms of performances. Bette to do all the computation in the crosshair component itself.

I can also think about someone who would like to highlight multiple values at once. But in that case, it would probably be better to create his own highlight component

In theory it's not that hard. In practice I don't see usecases.

What about charts with multiple X axes? Can't there be more than one axis highlight at a time?

I can also think about someone who would like to highlight multiple values at once. But in that case, it would probably be better to create his own highlight component

Would that be something that's easy to do? Should we have a demo for it?

I suspect it's somewhat useful. For example, you might want to click a button and highlight some points.

What about charts with multiple X axes? Can't there be more than one axis highlight at a time?

For me it looks like the worst case scenario with two axis highligh component rendered either on top of each other or fairly close

Would that be something that's easy to do? Should we have a demo for it?

That's a line or a rectangle to render. The logic should not be that different from what the demos in https://mui.com/x/react-charts/components/#scales

For me it looks like the worst case scenario with two axis highligh component rendered either on top of each other or fairly close

Sure, but shouldn't we support it? I think it's a bit weird that in controlled state the handler provides an array of values and the state itself only accepts a single value. IMO, both should accept an array or a single value.

In this case, since there can be multiple axes, I think we should always use an array. What downsides do you see in this?

Why do we now need to skip this test?