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

Refactor AxisSpec to be more aware of which domain it is representative #242

Open
1 task done
Tracked by #2320
emmacunningham opened this issue Jun 14, 2019 · 0 comments
Open
1 task done
Tracked by #2320
Labels
:axis Axis related issue discuss To be discussed enhancement New feature or request meta ...meta issue

Comments

@emmacunningham
Copy link
Contributor

@markov00 @nickofthyme here's an issue for us to discuss refactoring the AxisSpec.

Is your feature request related to a problem? Please describe.
Currently the way that the AxisSpec is implemented, it privileges the Position of the Axis, not the domain (x or y) to which the axis belongs. This causes a couple of problems already:

  • axes when rotated do not bring their titles with them and the titles are thus incorrect (see Axis titles remain in the same position when chart is rotated #131)
  • similarly, when rotated, if an axis has a specific tickFormatter, the formatter will be incorret (for example if you have a left y axis and bottom x axis with the left axis with an identity tick formatter and the bottom axis with a time formatter, on 90 or -90 degree rotation, the time formatter will be remain applied to the bottom axis with non-timestamp values)
  • also, when a custom domain is defined for an axis, if it is rotated 90 or -90 degrees, then the custom domain will be applied incorrectly (if at all)
  • this also impacts internal implementation as there are times when we need to know information about the x or y axes (see getAxesSpecForSpecId which is called within annotations, legend, and crosshairs computations) but always need to calculate based on rotation
  • this also potentially impacts future implementation of new chart types where a new chart type may not accomodate the current Positions available (Pie Chart for example)

Let's use this issue to discuss more how the current implementation may be impacting us, what we may need in the future, and take these into consideration as we develop a new API for defining an AxisSpec.

Describe the solution you'd like
TBD, would love to hear more from y'all

One thing that I think needs to happen is for us to make more explicit the relationship between an axis and a domain. This would help significantly with some of the other issues mentioned above. We could just add a domainType to AxisSpec, but we could also be more specific and create more specific XAxisSpec and YAxisSpec components which extend the basic AxisSpec (similar then to how we handle the SeriesSpecs).

Something else I've been thinking about is changing the name from AxisSpec to DomainSpec, thus we are privileging Domain as a important piece, not the Axis itself. One reason to possibly do this is that sometimes the user may want to configure something concerning a specific Domain (for example custom min/max DomainRange), but they may not want to render an Axis; the current workaround for this is to define an Axis with these domain configurations with the hide prop equal to true to not render the Axis. Another time that this comes up is that sometimes we need a formatter for values outside of the Axis (crosshair & annotation tooltips and the legend display value) but need to go through the Axis to get the tickFormatter. Changing from AxisSpec to DomainSpec would allow the user to define both a domain formatter as well as a tickFormatter for the Axis if they want separate formatters.

Our current AxisSpec definition:

/**
 * This spec describe the configuration for a chart axis.
 */
interface AxisSpec {
  /** The ID of the spec, generated via getSpecId method */
  id: AxisId;
  /** Style options for grid line */
  gridLineStyle?: GridLineConfig;
  /** The ID of the axis group, generated via getGroupId method
   * @default __global__
   */
  groupId: GroupId;
  /** Hide this axis */
  hide: boolean;
  /** shows all ticks, also the one from the overlapping labels */
  showOverlappingTicks: boolean;
  /** Shows all labels, also the overlapping ones */
  showOverlappingLabels: boolean;
  /** Shows grid lines for axis; default false */
  showGridLines?: boolean;
  /** Where the axis appear on the chart */
  position: Position;
  /** The length of the tick line */
  tickSize: number;
  /** The padding between the label and the tick */
  tickPadding: number;
  /** A function called to format each single tick label */
  tickFormat: TickFormatter;
  /** The degrees of rotation of the tick labels */
  tickLabelRotation?: number;
  /** The axis title */
  title?: string;
  /** If specified, it constrains the domain for these values */
  domain?: DomainRange;
}

If we move to defining the top-level spec as DomainSpec, that might look something like:

interface DomainSpec {
  id: DomainId;
  groupId: GroupId;
  axis?: AxisSpec;
  grid?: GridSpec;
  domainRange?: DomainRange;
  domainFormat?: ValueFormatter;
  domainType: 'x' | 'y'; // need to consider how this could be used for other chart types
}

interface AxisSpec {
  id: AxisId; // not sure that we need to uniquely identify an AxisSpec 
  hide: boolean;
  showOverlappingTicks: boolean;
  showOverlappingLabels: boolean;
  position: Position;
  tickSize: ValueFormatter;
  tickPadding: number;
  tickFormat: TickFormatter;
  tickLabelRotation?: number;
  title?: string;
}

interface GridSpec {
  gridLineStyle?: GridLineConfig;
  showGridLines?: boolean;
}

If the user defines no explicit X or Y domains, then we could fallback to the internal computed scale (xScale and yScales) configurations.

Describe alternatives you've considered

Additional context

Kibana Cross Issues
N/A

Checklist

  • this request is checked against already exist requests
    - [ ] 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
@emmacunningham emmacunningham added meta ...meta issue discuss To be discussed :axis Axis related issue enhancement New feature or request labels Jun 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:axis Axis related issue discuss To be discussed enhancement New feature or request meta ...meta issue
Projects
None yet
Development

No branches or pull requests

1 participant