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

area chart - horizontal indicator example #2796

Merged
merged 3 commits into from Sep 23, 2019

Conversation

@priley86
Copy link
Member

priley86 commented Aug 29, 2019

What:
Horizontal Indicator example for Area Charts. Currently reviewing this from a UX/design perspective.

Opened PF issue 2797 for this documentation.

Demo (Multi-color chart with threshold indicators and responsive container example):
https://patternfly-react-pr-2796.surge.sh/patternfly-4/charts/chartarea/

Current Questions:
* What color themes do we think appropriate for indicators?
* What stroke width do we find legible for indicators?
* Should indicators be connected lines or independent line segments?
* How should we handle mobile legends gracefully?
* Do we need custom threshold tooltip behavior?

Desktop:
chart-desktop

Tablet:
chart-tablet

Mobile:
mobile

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Aug 29, 2019

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

@tlabaj tlabaj requested a review from dlabrecq Aug 29, 2019
@priley86

This comment has been minimized.

Copy link
Member Author

priley86 commented Sep 4, 2019

linking #94 related (and started today in Insights Subscriptions)

@jeperry

This comment has been minimized.

Copy link
Member

jeperry commented Sep 4, 2019

(apologies for late and possibly misplaced comment) Not sure where to note this, but i think overlapping area charts with transparency makes this trickier. I'd like to see this applied to a stacked area chart as well. Speaking of which.. we don't seem to have one?

@dlabrecq

This comment has been minimized.

Copy link
Member

dlabrecq commented Sep 4, 2019

Note that a threshold line can be applied on top of other areas / lines, grids, etc. to be more visible. It's simply a matter of ordering.

We don't have a stacked area chart example, but that would be easy to create. Simply wrap the ChartArea components with ChartStack.

@mceledonia

This comment has been minimized.

Copy link

mceledonia commented Sep 5, 2019

Hey, working on getting a stacked chart example for you here, and I'll try to answer those questions:


What color themes do we think appropriate for indicators?

  • I guess this depends on the context of the indicators... If they're directly related to the values like above, I'd make them the same color but with a dashed line exactly as you have. If they're not, I almost feel like they should be treated the same as the values, just the next colors in the color order. So if you have three values and 2 threshold indicators, you'd have something like this:

Value 1 = Color 1
Value 2 = Color 2
Value 3 = Color 3
Threshold 1 = Color 4
Threshold 2 = Color 5

Is something like the above possible?


What stroke width do we find legible for indicators?

  • It looks like you have 2px which should be a good choice here. What variables can we control for the dotted line with victory? i.e. do we have control over dash length + gap size?

Should indicators be connected lines or independent line segments?

  • I'm not sure I'm following this one, but I like the dashed line style which differentiates from the solid line style we use for values in line charts.

How should we handle mobile legends gracefully?

  • I'd be curious if victory offers any solutions or has any examples of solutions for this. It seems like a problem universal to charts in general, but there's definitely a few solutions we could explore. One is a simple wrapping list under the chart, similar to what you have above in the mobile example but "birds threshold" would wrap to a second line.
    example:
    image

Do we need custom threshold tooltip behavior?

  • I don't think it would hurt for thresholds to have the same tooltip behavior as the values do, but if that's a technical challenge I don't think it's necessary by any means.

image

@dlabrecq

This comment has been minimized.

Copy link
Member

dlabrecq commented Sep 5, 2019

Considering the threshold is just another line, the ChartLine component would simply apply the next color in the color scale / family. Threshold colors are explicitly set in this example.

I believe we can customize the dash length and gap size. However, if a threshold requires different properties, we may need to consider creating a new component?

Regarding the dashed lines, Cost Management uses both solid and dashed lines to show the difference between the current and previous month's data. Can the dash symbol be optional depending on your usage? Perhaps we can suggest possible alternatives?

Custom tooltips are a definitely a technical challenge. What did you have in mind?

@jeperry

This comment has been minimized.

Copy link
Member

jeperry commented Sep 5, 2019

This looks really great, and the proposed rules @mceledonia has here would work for the use case I am aware of for Subscriptions UI (one threshold line not specifically related to any of the 2-3 area values).

@mceledonia

This comment has been minimized.

Copy link

mceledonia commented Sep 5, 2019

@dlabrecq I don't think the tooltips are necessary for the thresholds (chime in if anyone disagrees here) but figured if they're easy to toss in, they wouldn't hurt. I'm thinking the juice isn't worth the squeeze there.

For the dashed lines, since we have control over gap length and size, it's possible we recommend a dash style for values and a dash style for thresholds. They can be distinguishable enough to work I think, here's a quick example:

image

@dlabrecq

This comment has been minimized.

Copy link
Member

dlabrecq commented Sep 5, 2019

@mceledonia We can easily add a tooltip for thresholds -- no problem there. I just thought you were hinting at something different about the tooltip layout.

A different dashed line can be implemented for thresholds. Although, we should create a new component in that case, so everyone could apply the same threshold styling. We may also need to create a new legend symbol? I created the dash myself for use in Cost Management -- not easy to customize.

@priley86 priley86 force-pushed the priley86:horizontal-indicator branch from 9dfe3b1 to b764cf4 Sep 6, 2019
@priley86

This comment has been minimized.

Copy link
Member Author

priley86 commented Sep 6, 2019

i've started a ChartThreshold component just now alongside this PR. No visual changes yet for the dash style, but it would be defaulted inside the ChartThreshold component and overridable by the consumer.

@dlabrecq @cdcabrera - please weigh in and give a bit more specific feedback here once ready. I am happy to move forward however best!

@dlabrecq

This comment has been minimized.

Copy link
Member

dlabrecq commented Sep 7, 2019

IMO, we need a different dash style for thresholds. That's the main reason for creating a new ChartThreshold component (i.e., so users won't need to override styling).

Users won't be able to override the dash symbol for legends. We must create a new legend symbol for thresholds that folks can easily apply.

//cc @mceledonia

@dlabrecq

This comment has been minimized.

Copy link
Member

dlabrecq commented Sep 9, 2019

Took the liberty of creating a POC, showing how to create a new legend symbol for thresholds. You can find the code here: #2884

New threshold symbol
Screen Shot 2019-09-08 at 10 57 07 PM

Existing dash symbol
Screen Shot 2019-09-08 at 11 04 14 PM

@priley86

This comment has been minimized.

Copy link
Member Author

priley86 commented Sep 9, 2019

ok, after chatting w/ @dlabrecq - the proposal is to use the new threshold legend symbol created in #2884 instead of dash symbol. Also, we can move ChartThreshold towards a theme for adding the default strokeDashArray and add a getThresholdTheme function (similar to ChartDonut's getDonutTheme function here)

@priley86 priley86 force-pushed the priley86:horizontal-indicator branch from b764cf4 to 9ded3ad Sep 19, 2019
}
height={250}
padding={{
bottom: 100, // Adjusted to accomodate legend

This comment has been minimized.

Copy link
@priley86

priley86 Sep 19, 2019

Author Member

adding some additional height in new container class area-chart-threshold-bottom-responsive and inside the legend bottom padding to accommodate multi row legend

...rest
}: ChartLineProps) => {
const theme = getThresholdTheme(themeColor, themeVariant);
return (

This comment has been minimized.

Copy link
@priley86

priley86 Sep 19, 2019

Author Member

when extending ChartLineProps this way, it seems style prop will override theme. This method works for allowing theme colors, but would prevent the consumer from override style and theme directly. Any suggestions @dlabrecq ? I think this is close to what we discussed...

This comment has been minimized.

Copy link
@dlabrecq

dlabrecq Sep 23, 2019

Member

I believe this is expected behavior. When overriding the padding prop; for example, users must provide all padding variables; top, bottom, left, right.

@priley86 priley86 force-pushed the priley86:horizontal-indicator branch from 161f941 to 86548a5 Sep 19, 2019
line: {
style: {
data: {
strokeDasharray: '6,6'

This comment has been minimized.

Copy link
@priley86

priley86 Sep 19, 2019

Author Member

updated to 6,6

@priley86

This comment has been minimized.

Copy link
Member Author

priley86 commented Sep 19, 2019

thanks for the extensive feedback, and finally circling back here....here's my updates on this:

  • added new threshold symbol for legend
  • made legend multi-line using itemsPerRow
  • added new ChartThresholdTheme and made default strokeDasharray 6,6 instead of 3,3
  • rebased latest Victory updates here
  • updated screenshots above

Noted that this should still be safe to consume later in Subscriptions (current usage here):
https://github.com/RedHatInsights/curiosity-frontend/blob/ci/src/components/chartArea/chartArea.js#L216

priley86 added 2 commits Sep 19, 2019
{ name: 'Birds Threshold', x: 5, y: 3 }
]}
themeColor={ChartThemeColor.orange}
themeVariant={ChartThemeVariant.light}

This comment has been minimized.

Copy link
@dlabrecq

dlabrecq Sep 23, 2019

Member

You can remove the themeVariant, it defaults to ChartThemeVariant.light

...rest
}: ChartLineProps) => {
const theme = getThresholdTheme(themeColor, themeVariant);
return (

This comment has been minimized.

Copy link
@dlabrecq

dlabrecq Sep 23, 2019

Member

I believe this is expected behavior. When overriding the padding prop; for example, users must provide all padding variables; top, bottom, left, right.

@tlabaj
tlabaj approved these changes Sep 23, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@mceledonia

This comment has been minimized.

Copy link

mceledonia commented Sep 23, 2019

LGTM!

@tlabaj tlabaj merged commit be2a2a4 into patternfly:master Sep 23, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.