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.

Sign up for GitHub

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

Jump to bottom

Vislib/axis #8174

Closed
wants to merge 6 commits into from
Closed

Vislib/axis #8174

wants to merge 6 commits into from

Conversation

ppisljar
Copy link
Member

@ppisljar ppisljar commented Sep 7, 2016 •
edited
Loading

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.

replaces #7961

@ppisljar ppisljar added review Feature:Vislib Vislib chart implementation labels Sep 7, 2016
@spalger
Copy link
Contributor

spalger commented Sep 8, 2016

I want to start off by saying that many of my recommendations may contradict the style used in the vislib currently, hope that works for you 😸

@spalger
Copy link
Contributor

spalger commented Sep 8, 2016

I'm digging into the AxisScale and AxisLabels classes, and I think we should go ahead and try splitting fe4c318ca44f8dfd6832865b5e2ac138896bf297 up and improving the classes at the same time.

I have a few ideas about how this could go:

  • I'm thinking the new commits will go something like this:

    1. es6-ify XAxis and YAxis
    2. extract axis options from XAxis and YAxis into AxisOptions
    3. extract shared scale logic from XAxis and YAxis into AxisScale
    4. extract shared label locic from XAxis and YAxis into AxisLabels
    5. combine XAxis and YAxis into Axis

  • I think we should focus a bit on isolating the data used by the AxisScale and AxisLabels classes. Right now they are both a part of creating the Axis and require an instance of Axis. This is usually a sign that components are a bit too tightly coupled.

    Instead, lets pass the data that AxisScale needs into it's constructor and make it function without knowing anything about the Axis class.

  • Let's get rid of this.attr and this._attr, unless there is a reason for them that I'm not aware of.

spalger
spalger reviewed Sep 8, 2016
View reviewed changes
src/ui/public/vislib/lib/axis.js
const AxisTitle = Private(AxisTitleProvider);
const AxisLabels = Private(AxisLabelsProvider);
const AxisScale = Private(AxisScaleProvider);
const defaults = {
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about pulling these defaults into a AxisOptions class? I think the combination of writing these properties to this, and reading from there and this._attr is pretty confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

in my opinion it would make it more confusing ... maybe not with the axis itself ... but AxisLabels have their own config ... following same pattern we would then need AxisLabelsOptions as well ... and AxisTitleOptions ... and in the future i hope to do similar update to other parts of vislib where this would make even less sense. i think by defining defaults like this its quite clear what all the options that this class can handle are. let me know if you dissagree

Copy link
Contributor

Choose a reason for hiding this comment

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

My thought was that a single AxisConfig class could be used across the different Axis___ classes. I was thinking it would help document all of the options in one place, and also would help pass necessary configuration values into AxisScale and such.

@ppisljar
Copy link
Member Author

ppisljar commented Sep 8, 2016 •
edited
Loading

regarding this._attr and this.attr ....

  • this._attr was used in the old classes
  • some places i renamed it to this.attr to follow our styleguide ...

however you probably noticed that then i tried to use this directly ... so instead of having classes store there properties in this._attr they store it directly onto this. i am not sure thats a good idea ... the reason for it would be that the code is easier to read (at least for me ... this.type compared to this._attr.type)

regarding isolating data ... i am using object composition here as it made the most sense to me. i dont think there exists a scenario for a AxisLabel without Axis ... or AxisScale without Axis (also no axis without axisscale actually). they are tightly coupled, so i dont see a problem couping them in code as well. also i think object composition fits well here as we are modeling relationship we can describe with HAS-A ... Axis HAS-A AxisScale and Has-A AxisLabels

regarding commits

i think i am gonna start fresh, with a new PR, which will just ES6ify whole vislib ... we can then start again from there. (maybe even rebase this PR on top of it)

@ppisljar
Copy link
Member Author

ppisljar commented Sep 8, 2016

i created #8186 which converts vislib to use ES6 syntax

@spalger
Copy link
Contributor

spalger commented Sep 8, 2016 •
edited
Loading

i am using object composition here as it made the most sense to me

I think that's the right approach, I would just like to see more boundaries between the objects.

i dont see a problem couping

In my eyes, the problem with coupling those classes comes with maintenance. But I'm having trouble coming up with a meaningful example.

@spalger spalger mentioned this pull request Sep 10, 2016
Merged
@ppisljar
joining x_axis and y_axis into a single class axis
bc4508d 
- splitting it into 3 subclasses (Axis, AxisLabels, AxisScale)
- converting to ES6 classes + style fixes
- adding more customization options
@ppisljar
updating handler to work with new Axis class
8edc0e9 
- allowing handler to have multiple category/value axes (array)
@ppisljar
converting axis_title to ES6 classes and making it work with new axis
a630025
@ppisljar
updating column layout to support left/right top/bottom positioning o…
3708697 
…f axis

- updating css min-widths to 1px (removing them breaks the code) as we dont want to reserve the space for axes that dont exist.
@ppisljar
updating axis splits to work with new layout + new axis
82f9087
@ppisljar
updating charts and dispatcher to work with new axis
f36da8d
@spalger
Copy link
Contributor

spalger commented Sep 13, 2016 •
edited
Loading

We're going to move to a more iterative approach and start doing small pull-requests to the vislib-axis-refactor branch, which will culminate in another huge pr like this.

@spalger spalger closed this Sep 13, 2016
@ppisljar ppisljar mentioned this pull request Sep 13, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@spalger spalger

Labels
Feature:Vislib Vislib chart implementation review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ppisljar @spalger