feat(VictoryZoomContainer): Give examples of Victory Zoom component to allow zo… by jcaianirh · Pull Request #1879 · patternfly/patternfly-react · GitHub

…om capability on charts.

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.

PatternFly-React preview: https://1879-pr-patternfly-react-patternfly.surge.sh

Codecov Report

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

@@            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 Looks like your jest test are failing.

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

@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?

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

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.

@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.

Looks good to me.

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

@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.

@dlabrecq added zoom prop to chart group

LGTM

rebase to fix tests

…om capability on charts. fix patternfly#1715

…er instead

@dlabrecq updated for tsx conversion. please review

LGTM

jcaianirh requested a review from dlabrecq April 29, 2019 16:57

dlabrecq suggested changes Apr 29, 2019

View reviewed changes

Comment thread packages/patternfly-4/react-charts/src/components/ChartZoomContainer/ChartZoomContainer.js Outdated

redallen previously requested changes Apr 30, 2019

View reviewed changes

Comment thread packages/patternfly-4/react-charts/src/components/ChartZoomContainer/ChartZoomContainer.js Outdated

jcaianirh force-pushed the addZoomChart branch from 280fad2 to 6ab8939 Compare May 6, 2019 16:31

tlabaj previously requested changes May 8, 2019

View reviewed changes

dlabrecq reviewed May 12, 2019

View reviewed changes

dgutride added the PF4 label May 14, 2019

dlabaj assigned dlabrecq May 17, 2019

jcaianirh force-pushed the addZoomChart branch from 6ab8939 to d07f174 Compare May 28, 2019 19:13

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 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 requested review from dgutride, dlabrecq, redallen and tlabaj May 29, 2019 13:15

dlabrecq reviewed May 29, 2019

View reviewed changes

dlabrecq previously approved these changes May 29, 2019

View reviewed changes

jcaianirh dismissed dlabrecq’s stale review via 0dc72a6 June 3, 2019 14:51

jcaianirh force-pushed the addZoomChart branch from 01f8f6e to 0dc72a6 Compare June 3, 2019 14:51

dlabrecq reviewed Jun 3, 2019

View reviewed changes

dlabrecq previously approved these changes Jun 3, 2019

View reviewed changes

jcaianirh requested a review from dlabrecq June 3, 2019 18:21

jcaianirh dismissed dlabrecq’s stale review via c999405 June 3, 2019 20:03

jcaianirh force-pushed the addZoomChart branch from a76b626 to c999405 Compare June 3, 2019 20:03

jcaianirh force-pushed the addZoomChart branch from c999405 to 64bb088 Compare June 4, 2019 14:31

jcaianirh added 10 commits June 4, 2019 10:42

feat(ZoomChartContainer): Wrap the Victory Zoom component to allow zo…

a6344c3

…om capability on charts. fix patternfly#1715

Add lodash merge to imports

f0dcd1e

Add role to zoom component. Add examples

dcb8a95

Remove ChartZoomController. Add examples of using VictoryZoomControll…

4ac4efe

…er instead

Remove ZoomContainer from all files

ce989ec

Add addZoom option for Charts to provide simple zoom capability

dfe2096

remove extra comment

a8f6289

add zoom to chartgroup

e47e952

add allowzoom to chartgroup

56331ac

fix import spacing

6242393

jcaianirh force-pushed the addZoomChart branch from 0e9e3ea to 6242393 Compare June 4, 2019 14:43

remove index.js

026b12b

dlabaj approved these changes Jun 4, 2019

View reviewed changes

dlabrecq approved these changes Jun 4, 2019

View reviewed changes

tlabaj reviewed Jun 5, 2019

View reviewed changes

tlabaj merged commit 235c174 into patternfly:master Jun 5, 2019

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

feat(VictoryZoomContainer): Give examples of Victory Zoom component to allow zo… by jcaianirh · Pull Request #1879 · patternfly/patternfly-react · GitHub

Uh oh!

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

feat(VictoryZoomContainer): Give examples of Victory Zoom component to allow zo…#1879tlabaj merged 11 commits intopatternfly:masterpatternfly/patternfly-react:masterfrom jcaianirh:addZoomChartjcaianirh/patternfly-react:addZoomChartCopy head branch name to clipboard

Conversation

jcaianirh commented Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patternfly-build commented Apr 29, 2019

Uh oh!

codecov-io commented Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

dlabrecq May 12, 2019

Choose a reason for hiding this comment

Uh oh!

dgutride May 13, 2019

Choose a reason for hiding this comment

Uh oh!

jcaianirh commented May 28, 2019

Uh oh!

dlabrecq left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcaianirh commented Jun 3, 2019

Uh oh!

dlabrecq left a comment

Choose a reason for hiding this comment

Uh oh!

jcaianirh commented Jun 3, 2019

Uh oh!

jcaianirh commented Jun 3, 2019

Uh oh!

dlabrecq left a comment

Choose a reason for hiding this comment

Uh oh!

jcaianirh commented Jun 4, 2019

Uh oh!

dlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

tlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

feat(VictoryZoomContainer): Give examples of Victory Zoom component to allow zo…#1879
tlabaj merged 11 commits into
patternfly:masterfrom
jcaianirh:addZoomChart

jcaianirh commented Apr 29, 2019 •
edited
Loading

codecov-io commented Apr 29, 2019 •
edited
Loading

dlabrecq left a comment •
edited
Loading