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

Added new Hexagon shape in the Network #3420

Merged
merged 7 commits into from
Sep 9, 2017
Merged

Conversation

rg2609
Copy link
Contributor

@rg2609 rg2609 commented Sep 6, 2017

Fixed #3418 and added Hexagon shape for nodes.

@wimrijnders
Copy link
Contributor

From the Travis build:

[07:50:48] Using gulpfile ~/build/almende/vis/gulpfile.js
[07:50:48] Starting 'lint'...
[07:50:53] 

/home/travis/build/almende/vis/lib/network/modules/components/nodes/shapes/Hexagon.js
   5:1   error  Missing JSDoc comment  require-jsdoc
   6:14  error  Missing JSDoc comment  require-jsdoc
  10:7   error  Missing JSDoc comment  require-jsdoc
  14:19  error  Missing JSDoc comment  require-jsdoc

Please fix all error messages from gulp lint.

*/
CanvasRenderingContext2D.prototype.hexagon = function (x, y, r) {
this.beginPath();
var sides = 6
Copy link
Contributor

Choose a reason for hiding this comment

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

;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

for (var i = 1; i < sides; i++) {
this.lineTo(x+r*Math.cos(a*i),y+r*Math.sin(a*i));
}
this.closePath();
Copy link
Contributor

Choose a reason for hiding this comment

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

Indent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@wimrijnders wimrijnders left a comment

Choose a reason for hiding this comment

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

Looking good. Some trivial code stuff.

@wimrijnders
Copy link
Contributor

Can you add hexagon to the documentation as well please?

In file docs/network/nodes.html, there are three lists of node shapes, for properties:

  • scaling
  • shape
  • size

Please add 'hexagon' to this list - anywhere you see square, hexagon must also be present.

wimrijnders
wimrijnders previously approved these changes Sep 6, 2017
Copy link
Contributor

@wimrijnders wimrijnders left a comment

Choose a reason for hiding this comment

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

Perfect

@wimrijnders
Copy link
Contributor

wimrijnders commented Sep 6, 2017

Travis checks are failing, but this is not your fault. It's a known issue (and it's driving me crazy).

If you want to do something about this, please add the following:

In file test/Network.test.js change function after() to:

  after(function() {
    try {
      this.jsdom_global();
    } catch(e) {
      if (e.message() === 'window is undefined') {
        console.warning("'" + e.message() + "' happened again");
      } else {
        throw e;
      }
    }
  });

This is totally unrelated to your changes. But it would be a good test to see if it helps Travis.

@wimrijnders
Copy link
Contributor

😞 Didn't help. But thanks for doing it anyway. Your PR is good, despite the failing test. Nothing to do with the PR.

@macleodbroad-wf
Copy link
Contributor

lgtm +1

@rg2609
Copy link
Contributor Author

rg2609 commented Sep 8, 2017

How much time will it take to merge this PR?

@wimrijnders
Copy link
Contributor

Not too much. Someone else will do it and it depends on his availability. I think it will happen this weekend.

@yotamberk yotamberk merged commit cb87892 into almende:develop Sep 9, 2017
@rg2609 rg2609 deleted the develop branch November 13, 2017 03:15
primozs pushed a commit to primozs/vis that referenced this pull request Jan 3, 2019
* added hexagon shape to the node.

* Updated function for hexagon shape.js

* Modifid the shape login for hexagon

* Remove this.translate from the Shape.js for hexagon

* updated hexagon draw logic

* Fixed code review comments and update the branch

* Updated changes in test.js
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.

4 participants