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

Update Vislib Axis #7961

Closed
wants to merge 18 commits into from
Closed

Conversation

ppisljar
Copy link
Member

@ppisljar ppisljar commented Aug 9, 2016

Update Vislib Axis classes

this PR creates some changes around Vislib.

  • it replaces YAxis and XAxis by a single Axis class. this allows us to easily switch them (to create horizontal column charts for example)
  • it puts AxisTitle inside of Axis (as they are dependant)
  • it makes Axis more customizable (labels, title, lines, position ... everything is customizable)
  • it splits Axis in smaller subclasses (Axis, AxisLabels, AxisTitle and AxisScale)
  • it makes it possible to add multiple axes to one chart (like multiple Y, multiple X .. one left one right .. two left ... you name it)

however none of the features were actually added to kibana. this is just a ground work to make that happen in the future. for now merging this PR should have 0 impact for users, charts should look and feel exactly the same and everything should work exactly the same.

its very possible that options don't work/don't work correctly yet (like setting categoryaxis position to top and yaxis position to right, setting weird fonts and rotations ....) i plan to tackle all that in the future when adding the features to kibana (when i'll have an easy way to check all the possible options kibana offers).

also i updated some other parts of the vislib to use the first axis for now (when you can add multiple). in the future i plan to do some changes to the charts as well to allow using multiple axes and some other things (like having different chart types together (line + bar )

i didn't yet update/write tests, i would first like to get some opinions.

known issues:

  • endzones are shown where there shouldn't be
  • labels dont render correctly

else buckets++;
});

if (metrics < 2 || buckets !== 1) return false;
Copy link
Member Author

Choose a reason for hiding this comment

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

only show this option if we have 1 bucket only (doesn't make much sense with multiple buckets ...)

@Bargs
Copy link
Contributor

Bargs commented Aug 12, 2016

I'm happy to give this a review from a user perspective and a general code quality standpoint, but I'm not really qualified to say if this is the proper way to implement things in the vislib. You might need to grab @spalger for that.

@Bargs
Copy link
Contributor

Bargs commented Aug 12, 2016

There are some spacing issues in the current implementation:

screen shot 2016-08-12 at 2 00 27 pm

@Bargs
Copy link
Contributor

Bargs commented Aug 12, 2016

Seperate Y Axises should be Separate Y Axes

@Bargs
Copy link
Contributor

Bargs commented Aug 12, 2016

Do you think this would be useful for area charts at all?

@Bargs
Copy link
Contributor

Bargs commented Aug 12, 2016

I think we need some way to show which axis applies to which bar/line. Right now it's impossible to tell without comparing the size of the bars to the values in the tooltips.

@ppisljar
Copy link
Member Author

after the explanation Spencer gave on vislib i think im gonna change this implementation completely

@Bargs Bargs removed their assignment Aug 16, 2016
@epixa epixa removed their assignment Aug 17, 2016
- change yAxis to ValueAxis
- add more customization options to ValueAxis
- change yAxis to ValueAxis
- add more customization options to ValueAxis
- allow label rotation
- allow label truncate
- allow time domain
@ppisljar ppisljar changed the title Enh/multiple y axis Update Vislib Axis Aug 30, 2016
@ppisljar
Copy link
Member Author

i updated PR name and description ... please check on top


return this.axis;
Axis.prototype.getScale = function () {
return this.scale.scale;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this defined before this.createScale() is called?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why split this function in two?

})();

if (result === false) {
$scope.vis.params.splitYAxis = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a weird place to put this. Can you explain why it's here?

@ppisljar
Copy link
Member Author

ppisljar commented Sep 4, 2016

@spalger what do you think about classes being wrapped in factories here ? i don't see any real benefits, just unnecessary complication to the code ?

@spalger
Copy link
Contributor

spalger commented Sep 4, 2016

I think the reason it is done is so that they can be "angular-ized", which may not still be necessary but I bet it is. It is probably better that everything be in a Provider rather than just parts of the vislib, but maybe you disagree

@ppisljar ppisljar added the Feature:Vislib Vislib chart implementation label Sep 5, 2016
@ppisljar ppisljar mentioned this pull request Sep 7, 2016
@ppisljar ppisljar closed this Sep 7, 2016
Ikuni17 pushed a commit that referenced this pull request Aug 31, 2024
`v95.7.0` ⏩ `v95.9.0`

> [!CAUTION]
> This PR contains the final set of Emotion conversions for all EuiForm
components.
> If your plugin contains any custom CSS/styling to **EuiFormRow,
EuiFormControlLayout, EuiCheckbox, EuiRadio, or EuiSwitch**, ⚠️ make
sure to QA your UI to ensure no visual regressions have occurred! ⚠️

---

## [`v95.9.0`](https://github.com/elastic/eui/releases/v95.9.0)

- Updated `EuiSearchBar`'s optional `box.schema` prop with a new
`recognizedFields` configuration. This allows specifying the phrases
that will be parsed as field clauses
([#7960](elastic/eui#7960))
- Updated `EuiIcon` with a new `tokenSemanticText` glyph
([#7971](elastic/eui#7971))
- Added support for TypeScript 5
([#7980](elastic/eui#7980))

**Bug fixes**

- Fixed `EuiSelectableTemplateSitewide` styles when used within a
dark-themed `EuiHeader`
([#7977](elastic/eui#7977))

## [`v95.8.0`](https://github.com/elastic/eui/releases/v95.8.0)

- Updated `EuiHeaderLinks`'s mobile menu to set a slight popover padding
by default ([#7961](elastic/eui#7961))
- This can be overridden via `popoverProps.panelPaddingSize` if needed.
- Updated `EuiHeaderLink` to default to a size of `s` (down from `m`)
([#7961](elastic/eui#7961))

**Accessibility**

- Updated the `aria-label` attribute for the `EuiFieldSearch` clear
button ([#7970](elastic/eui#7970))

**Bug fixes**

- Fixed a visual bug with `<EuiDualRange showInput="inputWithPopover"
/>` form controls ([#7957](elastic/eui#7957))

**Deprecations**

- Deprecated `EuiFormRow`'s `columnCompressedSwitch` display prop. Use
`columnCompressed` instead, which will automatically account for child
`EuiSwitch`es ([#7968](elastic/eui#7968))
- Deprecated `EuiFormRow`'s `rowCompressed` display prop. Use `row`
instead for vertical forms, or `centerCompressed` for inline forms
([#7968](elastic/eui#7968))
- (Styling) Updated `EuiFormRow`'s `hasEmptySpaceLabel` prop to no
longer attempt to automatically align its content to a vertical center.
Use the `display="center"` prop for that instead
([#7968](elastic/eui#7968))

**CSS-in-JS conversions**

- Converted `EuiFormControlLayout` to Emotion
([#7954](elastic/eui#7954))
- Removed `.euiFormControlLayout--*icons` classNames and
`--eui-form-control-layout-icons-padding` CSS var. Use
`--euiFormControlRightIconsCount` or `--euiFormControlLeftIconsCount`
instead
- Converted `EuiFormLayoutDelimited` to Emotion
([#7957](elastic/eui#7957))
- Fixed `cloneElementWithCss` throwing an error when used multiple times
without a `key` prop ([#7957](elastic/eui#7957))
- Updated `cloneElementWithCss` utility to support a third argument that
allows prepending vs. appending the cloned Emotion css className
([#7957](elastic/eui#7957))
- Removed `@euiFormControlLayoutClearIcon` Sass mixin
([#7959](elastic/eui#7959))
- Converted `EuiDescribedFormGroup` to Emotion
([#7964](elastic/eui#7964))
- Converted `EuiForm`, `EuiFormHelpText`, and `EuiFormErrorText` to
Emotion ([#7966](elastic/eui#7966))
- Converted `EuiFormLabel` and `EuiFormLegend` to Emotion; Removed
`@euiFormLabel` mixin
([#7967](elastic/eui#7967))
- Converted `EuiFormRow` to Emotion
([#7968](elastic/eui#7968))
- Converted `EuiCheckbox` to Emotion
([#7969](elastic/eui#7969))
- Converted `EuiRadio` to Emotion
([#7969](elastic/eui#7969))
- Converted `EuiSwitch` to Emotion
([#7969](elastic/eui#7969))
- Removed the following Sass variables:
([#7969](elastic/eui#7969))
  - `$euiFormCustomControlDisabledIconColor`
  - `$euiFormCustomControlBorderColor`
  - `$euiRadioSize`
  - `$euiCheckBoxSize`
  - `$euiCheckboxBorderRadius`
  - `$euiSwitchHeight` (and compressed/mini variants)
  - `$euiSwitchWidth` (and compressed/mini variants)
  - `$euiSwitchThumbSize` (and compressed/mini variants)
  - `$euiSwitchIconHeight`
  - `$euiSwitchOffColor`
- Removed the following Sass mixins:
([#7969](elastic/eui#7969))
  - `euiIconBackground`
  - `euiCustomControl`
  - `euiCustomControlSelected`
  - `euiCustomControlDisabled`
  - `euiCustomControlFocused`

---------

Co-authored-by: Marta Bondyra <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Vislib Vislib chart implementation review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants