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

Specifying a groupId prop breaks x-axis tick formatting #134

Closed
2 tasks
sulemanof opened this issue Apr 1, 2019 · 3 comments
Closed
2 tasks

Specifying a groupId prop breaks x-axis tick formatting #134

sulemanof opened this issue Apr 1, 2019 · 3 comments
Labels
:axis Axis related issue bug Something isn't working :tooltip Related to hover tooltip :xy Bar/Line/Area chart related

Comments

@sulemanof
Copy link

Describe the bug
The chart shows a tooltip when hovering, which convert a time string in milliseconds into a readable time string via tickFormat prop in x-axis component.
image

It works fine unless I specify a groupId prop in a series.
image

The full code implementation could be found in this PR.

Expected behavior
X-axis formatting should not rely on a groupId field, or should have a possibility to switch this mode.

Screenshots
Take a look on the tooltip, time string isn't formatted anymore
tick-formatting-issue

Version (please complete the following information):

  • OS: Windows 10
  • Browser Chrome 73
  • Elastic Charts: 3.4.4

Kibana Cross Issues
Add any Kibana related issues here.

Checklist

  • every related Kibana issue is listed under Kibana Cross Issues list
  • kibana cross issue tag is associated to the issue if any kibana cross issue is present
@sulemanof sulemanof added the bug Something isn't working label Apr 1, 2019
@markov00 markov00 added this to the 7.1 milestone Apr 1, 2019
@markov00
Copy link
Member

markov00 commented Apr 1, 2019

Hey @sulemanof the groupId is necessary for axis to work when you want to have multiple series to appear on multiple different Y Axis.
Currently we, wrongly, apply this also to the X axis in this method

export function getAxesSpecForSpecId(axesSpecs: Map<AxisId, AxisSpec>, groupId: GroupId) {
let xAxis;
let yAxis;
for (const axisSpec of axesSpecs.values()) {
if (axisSpec.groupId !== groupId) {
continue;
}
if (isVertical(axisSpec.position)) {
yAxis = axisSpec;
} else {
xAxis = axisSpec;
}
}
return {
xAxis,
yAxis,
};
}

@emmacunningham I can for sure fix this having the horizontal axis un-linked from the groupId but I'm wondering if we can find a better way to specify the relation between X values and horizontal axis, as we are also going to revisit the concept of axis position in #131 maybe we can reward the props as the follows

<BarSpec xGroupId="xgroup"  yGroupId="yId1" />
<BarSpec xGroupId="xgroup"  yGroupId="yId2" />
<Axis groupId="xgroup" position="bottom" />
<Axis groupId="ygroup1" position="left" />
<Axis groupId="ygroup2" position="left" />
// or using a default xgroupId
<BarSpec yGroupId="yId1" />
<BarSpec yGroupId="yId2" />
<Axis position="bottom" />
<Axis groupId="ygroup1" position="left" />
<Axis groupId="ygroup2" position="left" />
// or we can just keep the groupId, limit to only 1 x axis group and have something like
// this that doesn't change the API
<BarSpec groupId="yId1" />
<BarSpec groupId="yId2" />
<Axis position="bottom" /> // when not specified and the position is bottom or top we are on x axis
<Axis groupId="ygroup1" position="left" />
<Axis groupId="ygroup2" position="left" />

@emmacunningham
Copy link
Contributor

emmacunningham commented Apr 2, 2019

Yes, I think the related issue is what an axis position means in terms of the chart. Currently, when a user specifies an axis and they specify the position, we infer what should be on the axis based on other factors in the chart (mostly chartRotation). This means that the meaning of the axis will change depending on the rotation. For a 0-degree rotated chart, a left axis will correspond with a yDomain and a bottom axis will correspond with an xDomain (that is, they will show the tick labels corresponding to values of those domains); for a 90-degree rotated chart, this changes such that the left axis will correspond with the xDomain and the bottom axis with a yDomain. I wonder if this is intuitive to users as usually when we think about axes and domains, we don't define them in terms of position but rather what we want them to represent (i.e. I don't refer to a left axis of a 0-degree rotated chart for a graph but I refer to the yAxis of a chart).

In our current system, it's easy to get confused about what the Axis represents because we define it in terms of Position, which then requires us to consider chartRotation and then think about what the meaning of the Axis could be.

Considering some of the options mentioned...

Specify x & y group IDs.

<BarSpec xGroupId="xgroup"  yGroupId="yId1" />
<BarSpec xGroupId="xgroup"  yGroupId="yId2" />
<Axis groupId="xgroup" position="bottom" />
<Axis groupId="ygroup1" position="left" />
<Axis groupId="ygroup2" position="left" />

This would still possibly allow a combination of props that should not be possible; for example, an xGroup ID for a vertical axis (where verticality is itself dependent on chartRotation). It's also not clear to me what the associated groupIDs mean (this is a carryover from our current implementation): once we rotate the chart, the groupId associations don't hold, by which I mean that for the above spec components, if we rotate the chart 90 degrees, then the left axes would show values belonging to the xDomain, while the bottom axis would show values corresponding to a yDomain. For example, see below:

axis_positions_1

Default xGroup ID.

// or using a default xgroupId
<BarSpec yGroupId="yId1" />
<BarSpec yGroupId="yId2" />
<Axis position="bottom" />
<Axis groupId="ygroup1" position="left" />
<Axis groupId="ygroup2" position="left" />

This seems preferable to the above at least because it locks in the default xGroupId; then perhaps groupId on an Axis means something like "if the chart is rotated and I represent an yDomain, this is the groupId for the yScale I'm associated with; if the chart is rotated and I reprensent an xDomain, then there is only one xScale to be associated with".

Limiting to 1-to-1 group-to-axis definition.

// or we can just keep the groupId, limit to only 1 x axis group and have something like
// this that doesn't change the API
<BarSpec groupId="yId1" />
<BarSpec groupId="yId2" />
<Axis position="bottom" /> // when not specified and the position is bottom or top we are on x axis
<Axis groupId="ygroup1" position="left" />
<Axis groupId="ygroup2" position="left" />

I think we still run into the same issues with rotation. Further, there is another problem currrently where, if there are two specs with different yScales and the chart is rotated, we lose the split axes. This then leads to some series not being represented fully (the line series below has a max value of 10, but we lose that in the rotation):

axis_positions_1

I wonder if, given that we can see how position and rotation interact currently, we might want to think about axis position slightly differently (and how it corresponds to a scale defined by a groupId).

type Axis = XAxis | YAxis;

// these aren't great names because they assume a norm for start/end which I'm not sure is universal
// especially since this is somewhat assuming reading order, but it serves to show that we can define 
// position as a binary independent of orientation/rotation
enum AxisPosition = {
  start = 'start',
  end = 'end',
}

enum ScaleType = {
  x = 'x',
  y = 'y',
}

interface XAxis {
  scaleType: ScaleType.x;
  // groupId not necessary
  position: AxisPosition; // positions based on rotation 
  //... other axis props
}

interface YAxis {
  scaleType: ScaleType.y;
  groupId: GroupId; // used to get relevant yScale
  position: AxisPosition; // positions based on rotation 
  //... other axis props
}

<BarSpec yGroupId="yId1" />
<BarSpec yGroupId="yId2" />
<Axis scaleType="x" position="end" /> // Bottom Axis on a 0-rotated chart
<Axis scaleType="y" yGroupId="ygroup1" position="start" /> // Left Axis on a 0-rotated chart
<Axis scaleType="y" yGroupId="ygroup2" position="end" /> // Right Axis on a 0-rotated chart

Rather than specify position as one of the directions, we give them a binary value so for an xScale type, the axis will either be at the Top ('start') or Bottom ('end') when the chart has a horizontal rotation; when rotated vertically, then the axis will either be Left ('start') or Right ('end'). Thus we have a way to rotate the axis and maintain the association with the scale. An axis is always defined relative to a scaleType (& groupId for yDomains) and the domain values for an axis are independent of its position.

@markov00 markov00 modified the milestones: Kibana 7.2, Kibana 7.3 May 22, 2019
@markov00 markov00 added :axis Axis related issue :tooltip Related to hover tooltip :xy Bar/Line/Area chart related labels Mar 26, 2020
@markov00 markov00 removed this from the Kibana 7.3 milestone Aug 26, 2020
@nickofthyme
Copy link
Collaborator

Duplicate of #676

@nickofthyme nickofthyme marked this as a duplicate of #676 Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:axis Axis related issue bug Something isn't working :tooltip Related to hover tooltip :xy Bar/Line/Area chart related
Projects
None yet
Development

No branches or pull requests

4 participants