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

feat(VictoryZoomContainer): Give examples of Victory Zoom component to allow zo… #1879

Merged
merged 11 commits into from Jun 5, 2019

Conversation

@jcaianirh
Copy link
Contributor

jcaianirh commented Apr 29, 2019

…om capability on charts.

fix #1715

What: Add examples on how to use VictoryZoomContainer. Add this component to the highest level Chart component as the containerComponent property to create a zoom-able chart.

@jcaianirh jcaianirh requested a review from dlabrecq Apr 29, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Apr 29, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Apr 29, 2019

Codecov Report

Merging #1879 into master will decrease coverage by 0.03%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1879      +/-   ##
==========================================
- Coverage   80.51%   80.48%   -0.04%     
==========================================
  Files         650      650              
  Lines        8234     8238       +4     
  Branches      610      614       +4     
==========================================
  Hits         6630     6630              
  Misses       1308     1308              
- Partials      296      300       +4
Flag Coverage Δ
#patternfly3 85.22% <ø> (ø) ⬆️
#patternfly4 75.73% <0%> (-0.08%) ⬇️
#patternflymisc 95.68% <ø> (ø) ⬆️
Impacted Files Coverage Δ
...ct-charts/src/components/ChartGroup/ChartGroup.tsx 82.35% <0%> (-10.99%) ⬇️
...nfly-4/react-charts/src/components/Chart/Chart.tsx 78.57% <0%> (-13.1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb29aa5...026b12b. Read the comment docs.

@jcaianirh jcaianirh force-pushed the jcaianirh:addZoomChart branch from 280fad2 to 6ab8939 May 6, 2019
Copy link
Contributor

tlabaj left a comment

@jcaianirh Looks like your jest test are failing.

import React from 'react';
import hoistNonReactStatics from 'hoist-non-react-statics';
import { VictoryZoomContainer } from 'victory';
import { merge } from 'lodash';

This comment has been minimized.

Copy link
@dlabrecq

dlabrecq May 12, 2019

Member

Where are we picking up the lodash package? I don't see it in the react-core dependencies.

This comment has been minimized.

Copy link
@dgutride

dgutride May 13, 2019

Member

@jcaianirh - I don't believe we currently use lodash in react-charts. We generally prefer pure es6/typescript methods instead of shelling out to a library for things like this - can you try to use native functionality instead?

@dgutride dgutride added the PF4 label May 14, 2019
@jcaianirh jcaianirh force-pushed the jcaianirh:addZoomChart branch from 6ab8939 to d07f174 May 28, 2019
@jcaianirh

This comment has been minimized.

Copy link
Contributor Author

jcaianirh commented May 28, 2019

@dlabrecq removed the ChartZoomController as we discussed and provided examples using VictoryZoomController for Bar, Line, and Stack charts.

@jcaianirh jcaianirh changed the title feat(ZoomChartContainer): Wrap the Victory Zoom component to allow zo… feat(VictoryChartContainer): Give examples of Victory Zoom component to allow zo… May 28, 2019
@jcaianirh jcaianirh changed the title feat(VictoryChartContainer): Give examples of Victory Zoom component to allow zo… feat(VictoryZoomContainer): Give examples of Victory Zoom component to allow zo… May 28, 2019
@jcaianirh jcaianirh requested review from dlabrecq, tlabaj, redallen and dgutride May 29, 2019
Copy link
Member

dlabrecq left a comment

The examples look good.

Just a thought... We might consider adding allowZoom prop to some components. Thus, we could add the VictoryZoomComponent for devs. If something more advanced is required, they could still override the containerComponent prop.

@jcaianirh jcaianirh force-pushed the jcaianirh:addZoomChart branch from 01f8f6e to 0dc72a6 Jun 3, 2019
@jcaianirh

This comment has been minimized.

Copy link
Contributor Author

jcaianirh commented Jun 3, 2019

@dlabrecq I added the addZoom prop to the pf Chart component, and updated the examples to use the 'convenience' allowZoom prop to show how to use it as well as provide the more complex example using the containerComponent.

Copy link
Member

dlabrecq left a comment

Looks good to me.

Just wondering if VictoryZoomContainer could be added to ChartGroup as well? Or, does this only make sense for Chart?

@jcaianirh

This comment has been minimized.

Copy link
Contributor Author

jcaianirh commented Jun 3, 2019

@dlabrecq at first I thought no, but now that I look at victory, they do provide an example of using ChartGroup without Chart, so I'm gonna add it in.

@jcaianirh

This comment has been minimized.

Copy link
Contributor Author

jcaianirh commented Jun 3, 2019

@dlabrecq added zoom prop to chart group

Copy link
Member

dlabrecq left a comment

LGTM

@jcaianirh jcaianirh requested a review from dlabrecq Jun 3, 2019
@jcaianirh jcaianirh force-pushed the jcaianirh:addZoomChart branch from a76b626 to c999405 Jun 3, 2019
@jcaianirh jcaianirh dismissed stale reviews from tlabaj and redallen Jun 3, 2019

rebase to fix tests

@jcaianirh jcaianirh force-pushed the jcaianirh:addZoomChart branch from c999405 to 64bb088 Jun 4, 2019
@jcaianirh jcaianirh force-pushed the jcaianirh:addZoomChart branch from 0e9e3ea to 6242393 Jun 4, 2019
@jcaianirh

This comment has been minimized.

Copy link
Contributor Author

jcaianirh commented Jun 4, 2019

@dlabrecq updated for tsx conversion. please review

@dlabaj
dlabaj approved these changes Jun 4, 2019
Copy link
Contributor

dlabaj left a comment

LGTM

Copy link
Contributor

tlabaj left a comment

LGTM

@tlabaj tlabaj merged commit 235c174 into patternfly:master Jun 5, 2019
2 checks passed
2 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.