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

Created ContentClipPath #1351

Merged
merged 5 commits into from
Jun 7, 2020
Merged

Created ContentClipPath #1351

merged 5 commits into from
Jun 7, 2020

Conversation

Xiot
Copy link
Contributor

@Xiot Xiot commented Jun 6, 2020

The ContentClipPath creates a new clipPath that is the size of the
inner portion of the graph.
Other series can use this with `style={{clipPath: 'url(#content-area)'}}

Addresses #1350

The ContentClipPath creates a new clipPath that is the size of the
inner portion of the graph.
Other series can use this with `style={{clipPath: 'url(#content-area)'}}
@mcnuttandrew
Copy link
Contributor

This is neat! Will you add some docs and some tests?

@Xiot
Copy link
Contributor Author

Xiot commented Jun 6, 2020

Sure. Although the tests will be pretty basic since the actual functionality depends on the svg handling of the browser.

@@ -0,0 +1,20 @@
import React from 'react';
import {shallow} from 'enzyme';
import ContentClipPath from '../../src/plot/content-clip-path';
Copy link
Contributor

Choose a reason for hiding this comment

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

the component tests in the library usually involves creating a showcase example (which then also promotes the feature) and then basically verifying that it loads correctly. I'm willing to bend away from this pattern (i'm not sure what our story with story book is anymore), but i think it's a really two birds one stone to replace instances like your shallow rendering of the wrapper with a concrete example?

Copy link
Contributor Author

@Xiot Xiot Jun 6, 2020

Choose a reason for hiding this comment

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

There is a separate storybook that I created that allows them toggle the clipping on and off.
I'm actually trying to break the dependency between the tests and the storybook as it requires some custom resolvers to ensure that the tests and and storybook point to the same version of react-vis.

Since its only possible to test this visually, I didn't see the need to create a XYPlot to place this in. Personally, I prefer shallow tests so that I can just test out the component that I'm using and not any of its dependencies, but I realize that has become somewhat of an anti-pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does the storybook actually get published anywhere, or is it just for developers to play with the charts locally?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it was meant to be published onto the website, but that that integration never got finished? Typically when i'm developing on react-vis I tend use the showcase. In fact I can't say if i've ever even opened the story book?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you are trying to keep storybook and tests seperate, this suggests that you should move that demo into the showcase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tend to use the storybook as I'm developing as I find that there is too much going on (number of graphs) in the showcase.
We eventually want to put more options and profiling tools inside of the storybook.

I'm actually working on another change to make development with the storybook even easier

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you are trying to keep storybook and tests seperate, this suggests that you should move that demo into the showcase?

I can put it in the showcase as well

@@ -0,0 +1,24 @@
## Clip

Depending on the data and domain, sometimes the series in the plot will extend into the axis. This can either be solved with a [Border](border.md), or the elements can be clipped.
Copy link
Contributor

@mcnuttandrew mcnuttandrew Jun 6, 2020

Choose a reason for hiding this comment

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

Maybe also point to some mdn docs about clipping for people aren't familiar?

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

const [clip, setClip] = useState(true);

return (
<>
Copy link
Contributor

Choose a reason for hiding this comment

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

i hadn't seen this syntax before, neat

Copy link
Contributor Author

@Xiot Xiot Jun 7, 2020

Choose a reason for hiding this comment

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

Which part? The array destructuring or the shorthand for React.Fragment?

@mcnuttandrew
Copy link
Contributor

Looks good! merge away

@Xiot Xiot merged commit cce5980 into uber:master Jun 7, 2020
@Xiot Xiot deleted the clip branch June 7, 2020 15:30
@mcnuttandrew
Copy link
Contributor

mcnuttandrew commented Jun 7, 2020 via email

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.

2 participants