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

Add AxisDisplayMode functionality #56

Merged
merged 2 commits into from
Jun 4, 2018
Merged

Add AxisDisplayMode functionality #56

merged 2 commits into from
Jun 4, 2018

Conversation

evancharlton
Copy link
Contributor

Add support for several y-axis display modes:

ALL: Show all of the axes.
NONE: Don't show any of the axes.
COLLAPSED: Hide the real axes and instead render a single generic axis
without values or color.

This also adds event listeners to AxisCollection, as specified via
onAxisMouseEnter / onAxisMouseExit properties on LineChart.

Copy link
Contributor

@martincognite martincognite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

if (axisDisplayMode === AxisDisplayMode.COLLAPSED) {
axes.push(this.renderPlaceholderAxis());
}
// We need to render all of the axes (even if they're hidden) in order to
Copy link
Contributor

@martincognite martincognite Jun 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This points to another problem about how state is kept and updated in the YAxis, i'll file an issue to remember this.

Add support for several y-axis display modes:

  ALL: Show all of the axes.
  NONE: Don't show any of the axes.
  COLLAPSED: Hide the real axes and instead render a single generic axis
             without values or color.

The yAxisDisplayMode property can be specified on an individual series
object, too. In this case, it overrides the one set on the LineChart
component, by rules of specificity.

This also adds event listeners to AxisCollection, as specified via
onAxisMouseEnter / onAxisMouseExit properties on LineChart.
@evancharlton
Copy link
Contributor Author

Merge conflicts resolved, and I also added partial collapsing, after feedback from @danlevings and @hamzaque . Please take another look, especially at the new stories.

@hamzaque
Copy link

hamzaque commented Jun 4, 2018

Is this still open for one more iteration?

// It might be nice to support zooming all of the axes when you zoom on
// the collapsed one. Experiment with this and either add it or remove
// all of this unused code.
zoomable: PropTypes.bool,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completely agree, having an all-series zoom feature through a collapsed axis would be really nice. It can be moved to another issue though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a new issue for this #66.

this.props.height
);
};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a strong feeling we could move common with YAxis component functionality into a separate component or a factory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is pretty hideous -- I want to fix all of this pixel-math in a separate PR.

}

renderPlaceholderAxis() {
const { axisDisplayMode, height, yAxisWidth, series } = this.props;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eslint screaming on axisDisplayMode and series unused variables, we should probably add pre commit hooks in a separate issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@evancharlton
Copy link
Contributor Author

@hamzaque yes, but depending on what the request is, we might push that to another PR. What's up?

@cognite-cicd
Copy link

[pr-server]
The storybook for this PR is hosted on https://griff-react-pr-56.surge.sh

@evancharlton evancharlton merged commit ed179de into 0.2.0 Jun 4, 2018
@evancharlton evancharlton deleted the merge-axis branch June 4, 2018 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants