AxisConfig by ppisljar · Pull Request #8275 · elastic/kibana

ppisljar · 2016-09-14T14:54:37Z

introducing AxisConfig class to handle all axis configuration, to help decouple Axis components from Axis. not complete yet but you can take a look.

…working

… vislib/axis-2

spalger · 2016-09-14T19:29:32Z

src/ui/public/vislib/lib/axis.js

-    isHorizontal() {
-      return (this.position === 'top' || this.position === 'bottom');
+      const elSelector = this.config.get('elSelector');
+      const rootEl = this.config.get('rootEl');


Just a note, it seems weird that elements can live in config. not sure why

yeah, i completely agree. however i think this should be changed at a later stage, when we start doing changes to the vis itself, for now i just made rootEl availible on config instead of going to vis.el

spalger · 2016-09-14T19:47:12Z

src/ui/public/vislib/lib/axis.js

        text.each(function textWidths() {
          lengths.push((() => {
-            if (self.isHorizontal()) {
+            if (self.config.isHorizontal()) {


If you prefetch this above you could almost get rid of self 😜

Which I hate for some reason

i hate self as well ... mostly with arrow functions we should be able to get rid of it ,.... however d3 (as far as i know) is kind of dependand on the old functions ... maybe thats not the case anymore, i'll check it out

spalger · 2016-09-14T19:48:59Z

src/ui/public/vislib/lib/axis.js

    draw() {
      const self = this;
+      const config = this.config;
+      const style = config.style;


Should this be config.get('style')?

spalger · 2016-09-14T19:49:55Z

src/ui/public/vislib/lib/axis.js

-              if (self.axisLabels) self.axisLabels.render(svg);
-              svg.call(self.adjustSize());
            }
+            if (self.axisLabels) self.axisLabels.render(svg);


this used to only happen if there was a container. Was that a bug?

spalger · 2016-09-14T19:53:45Z

src/ui/public/vislib/lib/axis_config.js

+  class AxisConfig {
+    constructor(config) {
+      if (config.type === 'category') {
+        _.defaultsDeep(config, categoryDefaults);


This is modifying the config variable, which probably isn't a good idea because the AxisConfig doesn't know where it came from or what it's purpose it.

We should probably start off the constructor with this._values = {} and then apply the defaults:

const typeDefaults = config.type === 'category' ? categoryDefaults : {} this._values = _.defaultsDeep({}, config, typeDefaults, defaults);

This also has the benefit of storing the config values under a private property, meaning that there is a single API to reading config, config.get('proerty')

spalger · 2016-09-14T19:55:00Z

src/ui/public/vislib/lib/axis_config.js

+    };
+
+    get(property) {
+      return _.get(this, property, null);


I think this should expose the ability to pass in a default

I also think it should verify that the property exists using _.has() and throw an error if it's not defined. That has proved pretty valuable on the server, and helps to ensure that changes to the defaults are updated everywhere they need to be.

how exactly would that work ? (has + defaults) ...
if we have a defaults when there isnt a value set we return default ...

or should we return error if value is not set at all, and only return default if value is set to undefined ?

spalger

This looks great, a couple changes requested, but should be pretty minor changes

spalger · 2016-09-14T20:02:26Z

src/ui/public/vislib/lib/axis_labels.js

-        this.filter = attr && attr.filter ? attr.filter : false;
-        this.rotate = attr && attr.rotate ? attr.rotate : 70;
+      if (this.config.isHorizontal() && this.config.isOrdinal()) {
+        this.filter = config.labels && config.labels.filter ? config.labels.filter : false;


these could be updated to use the config.get() function with the default argument:

this.filter = config.get('labels.filter', false)

spalger · 2016-09-14T20:16:51Z

src/ui/public/vislib/lib/axis_scale.js

    isPercentage() {
-      return (this.axis._attr.mode === 'percentage');
+      const mode = this.config.get('mode');
+      return (mode === 'percentage');


Go ahead and drop the parens around these

spalger · 2016-09-14T20:17:14Z

src/ui/public/vislib/lib/axis_scale.js


        if (isNaN(val)) throw new Error(val + ' is not a valid number');
-        if (this.axis.isPercentage() && this.axis._attr.setYExtents) return val / 100;
+        if (this.isPercentage() && this.isUserDefined()) return val / 100;


spalger · 2016-09-14T20:20:00Z

src/ui/public/vislib/lib/axis_scale.js

      // Prevents bars from going off the chart when the y extents are within the domain range
-      if (this.axis._attr.type === 'histogram' && this.scale.clamp) this.scale.clamp(true);
+      // todo: make this work (its accessing chart config ... we only have axis config availible)
+      //if (this.axis._attr.type === 'histogram' && this.scale.clamp) this.scale.clamp(true);


What is the purpose of scale.clamp? maybe there is a different indicator we could look for?

in case of histogram chart we want to make sure that the last column is the same size as the previous ones ...

spalger · 2016-09-14T20:20:27Z

src/ui/public/vislib/lib/axis_title.js

          const bbox = svg.append('text')
          .attr('transform', function () {
-            if (self.axis.isHorizontal()) {
+            if (self.config.isHorizontal()) {


spalger · 2016-09-14T20:22:57Z

src/ui/public/vislib/lib/axis.js

    getLength(el, n) {
-      if (this.isHorizontal()) {
-        return $(el).parent().width() / n - this._attr.margin.left - this._attr.margin.right - 50;
+      const margin = this.config.get('vis._attr.margin');


Is this accessing the entire vis? Or just the vis configuration? It feels like, if the vis has some property that should influence the axis it should pass it to the axis, rather than the axis reach out and read it from the vis.

its accessing vis config .... in the future we'll probably extract vis config in similar manner we did with axis config ?

spalger · 2016-09-14T20:24:28Z

hmm, not sure why some of the changes are under the review and some aren't, lame

ppisljar · 2016-09-15T12:49:27Z

most of the tests are now passing ...

ppisljar · 2016-09-15T14:43:18Z

tomorrow im gonna test this really well (comparing with old behaviour). i already found 2 things that are not working correctly, probably there are more.

wiggle option has no effect
percentage mode does not work correctly (the remaining tests are failing because of this)

spalger · 2016-09-16T00:09:25Z

Looking good

spalger

One last question, but I'm ready to merge

spalger · 2016-09-19T23:56:21Z

src/ui/public/vislib/lib/axis_labels.js

-            return config.get('labels.axisFormatter')(d);
+          if ((startPos + halfSize) < myPos && maxSize > (myPos + halfSize)) {
+            startPos = myPos + halfSize;
+            return this.innerHTML;


Is it a possible issue that we are setting the innerHTML to the text value of each tick? Should we be using textContent?

we are not setting it, just reading it. this part filters labels if there are to many, in case label should not be filtered it returns it AS IS ... so if your label would happen to include some html it would also work.

Ah, cool, thought that maybe .text() was setting the text value

ppisljar added 2 commits September 14, 2016 16:43

introducing AxisConfig class

1fd8f98

adding ordered and values back to axis to make other parts of vislib …

fe00e6f

…working

ppisljar added review Feature:Vislib Vislib chart implementation labels Sep 14, 2016

ppisljar assigned spalger Sep 14, 2016

ppisljar and others added 7 commits September 14, 2016 16:55

fixing style of axis.js (indentation)

a2d8f51

fixing few mistakes ...

5574d9e

fixing few mistakes ...

c054582

fixing few mistakes ...

978105a

renaming axis.scale to axis.axisScale

7c8e149

removing comments

8c82cac

Merge branch 'vislib-axis-refactor' of github.com:elastic/kibana into…

f25401e

… vislib/axis-2

spalger reviewed Sep 14, 2016

View reviewed changes

spalger suggested changes Sep 14, 2016

View reviewed changes

ppisljar added 13 commits September 15, 2016 10:13

fixing based on spalger comments

5e36e09

removing extend ErrorHandler from Axis subclasses

79aeedd

fixing indentation issue in dispatch.js

1015fbc

forgot to remove super calls

d6937d3

fixing tests

4257b75

adding scale type config

4f49b1f

fixing tests

942d6e9

fixing tests

444ec0c

fixing tests

a9d0a4b

fixing axisTitle tests

78f2566

fixing layout tests

65a616d

fixing layout tests

2131a69

fixing some more tests

b4da928

ppisljar added 2 commits September 15, 2016 15:46

fixing x-axis tests

93a5a96

fixing most of y-axis tests

77f9cdc

spalger approved these changes Sep 16, 2016

View reviewed changes

ppisljar added 2 commits September 16, 2016 12:01

fixing mismatches with current master

4ea7d29

fixing remaining tests

c4709f6

spalger reviewed Sep 19, 2016

View reviewed changes

spalger mentioned this pull request Sep 20, 2016

Enh/multiple chart types #8329

Merged

ppisljar merged commit 9148892 into elastic:vislib-axis-refactor Sep 20, 2016

ppisljar mentioned this pull request Nov 11, 2016

Vislib Point Series updates #9044

Merged

Conversation

ppisljar commented Sep 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spalger Sep 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spalger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spalger Sep 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spalger commented Sep 14, 2016

Uh oh!

ppisljar commented Sep 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ppisljar commented Sep 15, 2016

Uh oh!

spalger commented Sep 16, 2016

Uh oh!

spalger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments

ppisljar commented Sep 14, 2016 •

edited

Loading

spalger Sep 14, 2016 •

edited

Loading

spalger Sep 14, 2016 •

edited

Loading

ppisljar commented Sep 15, 2016 •

edited

Loading