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

Some quick EuiSeriesChart additions #1198

Merged
merged 7 commits into from
Sep 19, 2018
Merged

Some quick EuiSeriesChart additions #1198

merged 7 commits into from
Sep 19, 2018

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Sep 18, 2018

Summary

1. Suppressing non-EUI vis color warning

I commented out the console warning (and tests) that displayed if you passed a color that is not one of the EUI vis palette colors. Every chart render would spit out this warning and would really overload the console output. I only commented it for now to allow us to revisit if necessary.

2. Added some opacity options to line and area series’

I kept the defaults of 1 but this helps to be able to lessen the line series border and area series fill.

Ex:

screen shot 2018-09-18 at 12 38 08 pm

3. Using the visualize app logo for empty prompt and vertically centering

Also, changed some spacing to be conditional EuiEmptyPrompt (only add space between content and actions if both body and actions exist).

Before

screen shot 2018-09-18 at 12 22 00 pm

After

screen shot 2018-09-18 at 12 21 37 pm

Checklist

  • This was checked in mobile
  • This was checked in IE11
  • This was checked in dark mode (Yes, but charts still have dark mode issues)
  • Any props added have proper autodocs
    - [ ] Documentation examples were added
  • A changelog entry exists and is marked appropriately
    - [ ] This was checked for breaking changes and labeled appropriately
  • Jest tests were updated or added to match the most common scenarios
    - [ ] This was checked against keyboard-only and screenreader scenarios

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Looks good.

@@ -11,6 +11,7 @@ export function VisualizationColorType(props, propName) {
return new Error('Color must be a valid hex color string in the form #RRGGBB');
}
if (!VISUALIZATION_COLORS.includes(color.toUpperCase())) {
console.warn('Prefer safe EUI Visualization Colors.');
// Suppress warning for now as it can overwhelm the console for custom charts
// console.warn('Prefer safe EUI Visualization Colors.');
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's OK to just remove this. We'll end up using the palette service for something like this anyway.

@@ -4,6 +4,7 @@
- Fixed issue with unselected tabs and aria-controls attribute in EuiTabbedContent
- Added `tag` icon ([#1188](https://github.com/elastic/eui/pull/1188))
- Replaced `logging` app icon ([#1194](https://github.com/elastic/eui/pull/1194))
- Added some opacity options to `EuiLineSeries` and `EuiAreaSeries` ([#1198](https://github.com/elastic/eui/pull/1198))
Copy link
Contributor

Choose a reason for hiding this comment

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

@chandlerprall should we label these under the experimental subheader? Or only do that for experimental breaks?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with either. The chart themselves were/are flagged as experimental, and these changes themselves are not experimental in of themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that was kind of my line of thought too

onSeriesMouseOut={this._onSeriesMouseOut}
style={{
cursor: isMouseOverSeries && onSeriesClick ? 'pointer' : 'default',
opacity: fillOpacity,
Copy link
Contributor

Choose a reason for hiding this comment

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

react-viz's AreaSeries supports an opacity prop, same as their LineSeries. Did you try passing opacity through?

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 did, and it didn't work

Copy link
Contributor

Choose a reason for hiding this comment

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

I poked around, looks like react-viz overrides the opacity value for some series items somewhere in its internals, so that's cool.

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

LGTM; pulled and played with opacity in docs locally

@cchaos cchaos merged commit 1352a5b into elastic:master Sep 19, 2018
@cchaos cchaos deleted the vis-edits branch September 26, 2018 20:51
@snide snide mentioned this pull request Oct 3, 2018
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.

3 participants