Skip to content
This repository has been archived by the owner on Jul 29, 2019. It is now read-only.

Added structure for default values #2176

Merged
merged 3 commits into from
Oct 18, 2016
Merged

Conversation

wimrijnders
Copy link
Contributor

This adds the initial defaults structure. It currently only does the simple fields; the rest require a bit more work.

Sorry, my mind is not clear enough to do the rest right now. Will add it bit by bit.

@@ -147,15 +210,16 @@ function Graph3d(container, data, options) {
// TODO: customize axis range

// colors
this.axisColor = '#4D4D4D';
this.gridColor = '#D3D3D3';
this.dataColor = {
Copy link
Member

Choose a reason for hiding this comment

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

Why not move dataColor also to the DEFAULTS object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These need special handling

style:

    if (options.style !== undefined) {
      var styleNumber = this._getStyleNumber(options.style);
      if (styleNumber !== -1) {
        this.style = styleNumber;
      }
    }

dataColor:

  if (options.dataColor) {
      if (typeof options.dataColor === 'string') {
        this.dataColor.fill = options.dataColor;
        this.dataColor.stroke = options.dataColor;
      }
      else {
        if (options.dataColor.fill) {
          this.dataColor.fill = options.dataColor.fill;
        }
        if (options.dataColor.stroke) {
          this.dataColor.stroke = options.dataColor.stroke;
        }
        if (options.dataColor.strokeWidth !== undefined) {
          this.dataColor.strokeWidth = options.dataColor.strokeWidth;
        }
      }
    }

I only did the simple copyable fields right now.
Bear with me; I know what needs to be done.


Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you wait a moment, I can get xCenter and yCenter into the defaults.

this.zValueLabel = passValueFn;

this.filterLabel = 'time';
this.legendLabel = 'value';
this.showLegend = undefined; // auto by default (based on graph style)

this.style = Graph3d.STYLE.DOT;
Copy link
Member

Choose a reason for hiding this comment

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

What's with showLegend an style?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

showLegend is non-trivial:

  this.showLegend        =  (this.defaultShowLegend !== undefined) ? this.defaultShowLegend : isLegendGraphStyle;

@wimrijnders
Copy link
Contributor Author

Here you go; these were easy enough to do.

@wimrijnders
Copy link
Contributor Author

Look, I see the 'Changes requested' message; I hope you understand that the rest is a bit harder to do than simply copying over. I stated in the initial message that this change does the 'simple' fields only, and that the rest requires 'a bit more work'.

Let me put it this way: do you agree until now? If so, there are two options:

  1. Accept this PR and let me add the rest of the defaults incrementally in separate PR's.
  2. Leave this PR open and wait till the rest has been done.

Both are fine with me. I have a personal preference for incremental PR's, but keeping this one open works for me as well.

@wimrijnders
Copy link
Contributor Author

I would appreciate a response to previous in due time.

Also, I have to say I am a bit miffed that you slapped a 'Changes requested' on this PR without actually requesting changes. I really don't want to have to guess your thoughts.

Could you please explicitly state what changes you request? Thanks.

@yotamberk yotamberk merged commit 810922a into almende:develop Oct 18, 2016
@wimrijnders
Copy link
Contributor Author

So. You approved without adding a single word of response.

Alexander, I will state this explicitly because it is bothering me: I consider this non-communication of yours very, very bad form. Please change this.

@wimrijnders wimrijnders deleted the PR12 branch October 19, 2016 06:17
@mojoaxel
Copy link
Member

I consider this non-communication of yours very, very bad form. Please change this.

I'm very sorry! I'll try to communicate more.
Let me just say, it was late yesterday and actually I just agreed with you: "Accept this PR and let me add the rest of the defaults incrementally in separate PR's". That is why I just approved it. I should have just added "comments" and not "request changes". My mistake.
No bad atmosphere intended! ☮️

@wimrijnders
Copy link
Contributor Author

Thank you very much! I honestly appreciate this response. Time for emoticons:

👍 ☀️ 😸 🎵 🍻

@mojoaxel
Copy link
Member

mojoaxel commented Oct 19, 2016

@wimrijnders By the way: You are welcomed in our new chat-room (that sounds sooo 90s): https://gitter.im/vis-js/Lobby

@wimrijnders
Copy link
Contributor Author

Thank you, Unfortunately, the link is not working.

@mojoaxel
Copy link
Member

Thank you, Unfortunately, the link is not working.

I fixed it

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants