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(chart-stack): add monthly example #2625

Merged
merged 4 commits into from Aug 30, 2019

Conversation

@priley86
Copy link
Member

priley86 commented Aug 1, 2019

What:
Refactors downstream monthly stack chart view in Subscriptions based on current discussions. This needs more review and iteration, but we'd like to tackle this problem in PatternFly!

Note: will need to test whatever solution we come up w/ here downstream before merge...

Current screenshots...

https://patternfly-react-pr-2625.surge.sh/patternfly-4/charts/chartstack/

Desktop:
Screen Shot 2019-08-01 at 10 27 03 AM

Tablet:
Screen Shot 2019-08-01 at 10 27 54 AM

Mobile:
Screen Shot 2019-08-01 at 10 28 11 AM

Additional issues:

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Aug 1, 2019

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

@priley86 priley86 requested review from dlabrecq and mattnolting Aug 1, 2019
Copy link
Member

dlabrecq left a comment

Looks good @patrick. Nice addition to the current examples!

@priley86 priley86 mentioned this pull request Aug 1, 2019
4 of 4 tasks complete
@cdcabrera cdcabrera mentioned this pull request Aug 1, 2019
@priley86 priley86 added on hold ux review and removed on hold labels Aug 1, 2019
@priley86

This comment has been minimized.

Copy link
Member Author

priley86 commented Aug 2, 2019

after further testing downstream, this appears to broken on IPhone X:

Screen Shot 2019-08-02 at 1 44 30 PM

Seeing the same thing downstream in Insights, but this is much closer to the design in place here now:
RedHatInsights/curiosity-frontend#70

@dlabrecq

This comment has been minimized.

Copy link
Member

dlabrecq commented Aug 2, 2019

There's a fixLabelOverlap prop you can try. That reduces the number of tick labels to fit the length of the axis.

@priley86

This comment has been minimized.

Copy link
Member Author

priley86 commented Aug 2, 2019

Works for me...

Screen Shot 2019-08-02 at 2 53 07 PM

Copy link

bclarhk left a comment

The spacing on the desktop view looks pretty awkward. Can the column widths be adjusted as well?

@dlabrecq

This comment has been minimized.

Copy link
Member

dlabrecq commented Aug 5, 2019

Spacing is typically related to the overall container width. The wider the SVG, the more spacing between bars. However, the spacing between bars can also be adjusted via properties such as barRatio, barWidth, and domainPadding, etc.

If you prfefer to see bars grouped by moth or year (e.g., https://www.patternfly.org/v3/pattern-library/data-visualization/bar-chart/), then the data needs change. Currently, the chart is showing daily data.

@priley86

This comment has been minimized.

Copy link
Member Author

priley86 commented Aug 6, 2019

cc: @cdcabrera ^^

@priley86 priley86 requested a review from cdcabrera Aug 6, 2019
@priley86 priley86 force-pushed the priley86:monthly-stack-chart branch from d9200e1 to ca23273 Aug 13, 2019
Copy link
Member

cdcabrera left a comment

Like the removal of the setTimeout by using lifecycle instead. Last thing I would say is that this is more of a "daily" view rather than a "monthly", but could see it going either way.

We actually use a flavor of this layout within https://github.com/RedHatInsights/curiosity-frontend

@tlabaj tlabaj requested a review from mcarrano Aug 29, 2019
@mcarrano

This comment has been minimized.

Copy link
Member

mcarrano commented Aug 29, 2019

@mceledonia can you take a look and let me know if you see any issues? Thanks.

@mceledonia

This comment has been minimized.

Copy link

mceledonia commented Aug 29, 2019

This looks good to me, we should recommend avoiding the bars not having any space (min 1px) between them though! Fix label overlap looks good to me though.

@dlabaj
dlabaj approved these changes Aug 30, 2019
@tlabaj
tlabaj approved these changes Aug 30, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@tlabaj tlabaj merged commit 0a29e99 into patternfly:master Aug 30, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Aug 30, 2019

Your changes have been released in:

  • @patternfly/react-charts@4.9.0

Thanks for your contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.