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

Clean up bugs found in the 0.2.0 migration #64

Merged
merged 4 commits into from Jun 4, 2018
Merged

Conversation

evancharlton
Copy link
Contributor

Calculate domain based on y0/y1

If y0Accessor and y1Accessor are specified, use those to calculate the
domain for the chart.

Remove styled-components

This has disastrous consequences if the user of griff also uses
styled-components, and with a different version (virtually guaranteed).

sizing

This change also involved an attempt to move to CSS Grid to
automatically determine the components sizes, but without relying on
sizeMe. However, this was aborted for the time being, with some small
sizing remnants hiding in the code. One example is the new contextChart
config, which looks like this:

  contextChart: PropTypes.shape({
    // Whether to show the context chart at all.
    visible: PropTypes.bool,
    // How high (in pixels) the context chart should be. Note that an
    // extra 50 pixels will be added for its X axis.
    height: PropTypes.number,
    // How high (in percent of overall height) the context chart should
    // be. Note that 50 pixels is reserved for it X axis height.
    heightPct: PropTypes.number,
  }

Calculate domain based on y0/y1

If y0Accessor and y1Accessor are specified, use those to calculate the
domain for the chart.

Remove styled-components

This has disastrous consequences if the user of griff also uses
styled-components, and with a different version (virtually guaranteed).

This change also involved an attempt to move to CSS Grid to
automatically determine the components sizes, but without relying on
sizeMe. However, this was aborted for the time being, with some small
sizing remnants hiding in the code. One example is the new contextChart
config, which looks like this:

  contextChart: PropTypes.shape({
    // Whether to show the context chart at all.
    visible: PropTypes.bool,
    // How high (in pixels) the context chart should be. Note that an
    // extra 50 pixels will be added for its X axis.
    height: PropTypes.number,
    // How high (in percent of overall height) the context chart should
    // be. Note that 50 pixels is reserved for it X axis height.
    heightPct: PropTypes.number,
  }
@@ -27,8 +27,7 @@
"lodash.isequal": "^4.5.0",
"np": "^2.20.1",
"react-select": "^1.2.1",
"react-sizeme": "2.4.3",
"styled-components": "^3.2.6"
Copy link
Contributor

Choose a reason for hiding this comment

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

We can have it as a peer-dependency if we want. That is the correct way to solve the issue (given we'd like to keep styled-components that is).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only reason it was in there was for the brush handles, and now that we have a custom one, was can definitely kill this. However, that's good to know about peer dependencies!

`H${halfStrokeWidth}`,
// Draw a vertical line from bottom to top
`V${range[1]}`,
// Finish wwith another horizontal line
Copy link
Contributor

Choose a reason for hiding this comment

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

supernit: typo wwith

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

yAccessor={d => d[1]}
baseDomain={staticBaseDomain}
series={[
{ id: 10, color: 'steelblue', y0Accessor, y1Accessor },
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't seem to pick up the domain calculation here -- both series have the same base domain;

https://www.dropbox.com/s/3j3pf3eble6vb4l/Skjermbilde%202018-06-03%2021.44.15.png?dl=0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wow, well spotted! That would've been a nightmare to debug -- it was a rebase conflict.

@cognite-cicd
Copy link

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

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, very nice changes.

@evancharlton evancharlton merged commit eb43a6d into 0.2.0 Jun 4, 2018
@evancharlton evancharlton deleted the migration branch June 4, 2018 04:05
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

3 participants