Support branch label rotation in 'horizontal mode'. #134

cgddrd · 2016-12-26T22:04:27Z

This PR enables graphs rendered in 'horizontal mode' to support branch label rotations besides the default of -90 (270) degrees.

Whilst the motivation and focus of this work is to allow for branch labels to be set to zero degrees of rotation whilst rendered under 'horizontal mode', no limitations on the rotation angle are enforced. It should be noted however, that any label rendered with a rotation angle NOT set to an increment of 90 degrees (i.e 0, 90, 180 or 270) is not guaranteed to render in a sensible manner.

See below for an illustration of these changes using the example file 'gitflowsupport.js':

Before: labels auto-default to a rotation of -90 degrees in horizontal mode

Before: labels (manually) forced to render at 0 degrees (positioning issues)

After: labels configured to render at 0 degrees (positioning and alignment handled correctly)

Please note: These changes do not affect the current behaviour for rendering branch labels under 'vertical' or 'vertical-reverse' modes.

Configuration of the rotation angle for branch labels continues to be set using the existing template.branch.labelRotation property, as highlighted in the example below:

var graphConfig = new GitGraph.Template({
    branch: {
        color: "#000000",
        mergeStyle: "straight",
        labelRotation: 0
    },
});

var config = {
  template: graphConfig,
  mode: "extended",
  orientation: "horizontal"
};

…it.sha1)'.

… mode, plus fies for tag labels between vertical and horizontal modes.

…ting to acquire its value.

…thub.com/cgddrd/gitgraph.js into orientation-modes-support-label-rotation

cgddrd · 2016-12-26T22:13:38Z

Closed due to CI failure. Will fix and resubmit.

… properties.

…ackarrow' template definitions.

…n return values for condition checking).

…rrectly, and addressed JSHint warnings within '.every()' polyfill.

cgddrd · 2016-12-27T00:14:28Z

Re-opened having addressed failing tests and JSHint warnings.

nicoespeon · 2017-01-02T09:26:16Z

Hi @cgddrd!

Thanks a lot for this. This is awesome 👌

I'll take some time to review and test it in the following days, I'll let you know.
Still, this is awesome 😄

cgddrd · 2017-01-02T09:28:07Z

Hi @nicoespeon,

That sounds great! Thanks very much, looking forward to hearing back from you!

nicoespeon

Thanks a lot again for your submission. That's a really cool contribution!

I've a few requested changes, only minor stuff − some console.log and personal preferences.

Let me know when you're done so I can merge your contribution, add some docs and release it 👍

nicoespeon · 2017-01-02T17:30:03Z

src/gitgraph.js

@@ -45,7 +45,8 @@
    case "vertical-reverse" :
      this.template.commit.spacingY *= -1;
      this.orientation = "vertical-reverse";
-      this.template.branch.labelRotation = 0;
+      this.template.branch.labelRotation =  !(_isNullOrUndefined(options, "template.branch.labelRotation")) ?


I tend to avoid negated conditions for readability. Could you reverse this ternary?

nicoespeon · 2017-01-02T17:30:49Z

src/gitgraph.js

@@ -55,7 +56,8 @@
      this.template.commit.spacingY = 0;
      this.template.branch.spacingX = 0;
      this.orientation = "horizontal";
-      this.template.branch.labelRotation = -90;
+      this.template.branch.labelRotation =  !(_isNullOrUndefined(options, "template.branch.labelRotation")) ?


Same here, I prefer to have a "positive" condition than keeping "default value at the end".

nicoespeon · 2017-01-02T17:31:21Z

src/gitgraph.js

      this.template.commit.tag.spacingX = -this.template.commit.spacingY;
      this.template.commit.tag.spacingY = this.template.branch.spacingY;
      break;
    default:
      this.orientation = "vertical";
-      this.template.branch.labelRotation = 0;
+      console.log(options);
+      console.log(_isNullOrUndefined(options, "template.branch.labelRotation"));


Spotted remaining console.log 😉

Sorry, thought I'd cleared these out! Poor checking on my part! Removed.

nicoespeon · 2017-01-02T17:31:27Z

src/gitgraph.js

@@ -66,13 +68,17 @@
      this.template.commit.spacingY = 0;
      this.template.branch.spacingX = 0;
      this.orientation = "horizontal-reverse";
-      this.template.branch.labelRotation = 90;
+      this.template.branch.labelRotation =  !(_isNullOrUndefined(options, "template.branch.labelRotation")) ?


Same here =)

nicoespeon · 2017-01-02T17:31:33Z

src/gitgraph.js

-      this.template.branch.labelRotation = 0;
+      console.log(options);
+      console.log(_isNullOrUndefined(options, "template.branch.labelRotation"));
+      this.template.branch.labelRotation =  !(_isNullOrUndefined(options, "template.branch.labelRotation")) ?


Same here =)

Also removed.

nicoespeon · 2017-01-02T17:32:48Z

src/gitgraph.js

@@ -1388,6 +1439,7 @@
   * @private
   */
  function _drawTextBG ( context, x, y, text, color, font, angle, useStroke ) {
+


Was this blank line a typo?

Sorry, it's a habit of mine to add more whitespace when working on a function. Scheduled for removal.

nicoespeon · 2017-01-02T17:34:44Z

src/gitgraph.js

+        throw new TypeError("this is null or not defined");
+      }
+
+      // 1. Let O be the result of calling ToObject passing the this


Thanks for the detailed polyfill and the link to the source, that's helpful (and documented).

Still, you can get rid of all the detailed implementation comments as they mostly describe what the code is doing rather than why, which is not really useful.

Thanks a lot =)

Comments removed.

nicoespeon · 2017-01-02T17:35:06Z

src/gitgraph.js

  // Expose GitGraph object
  window.GitGraph = GitGraph;
  window.GitGraph.Branch = Branch;
  window.GitGraph.Commit = Commit;
  window.GitGraph.Template = Template;
+


Same here: was this blank line intended?

This was also force of habit on my part. Will be removed.

…support-label-rotation.

cgddrd · 2017-01-05T19:30:06Z

Hi @nicoespeon,

Apologies for the delay, I've now finished making the requested changes.

Let me know your thoughts.

Cheers!

…discussion for nicoespeon#132.

cgddrd · 2017-01-05T19:42:29Z

src/gitgraph.js

@@ -1088,7 +1085,7 @@
        var textHeight = _getFontHeight( this.tagFont );
        _drawTextBG( this.context,
          this.x - this.dotSize / 2,
-          ((this.parent.columnMax + 1) * this.template.commit.tag.spacingY) - this.template.commit.tag.spacingY / 2 + (this.parent.tagNum % 2) * textHeight * 1.5,


This change was not meant to be committed. I have since reversed this change and recommitted.

cgddrd · 2017-01-05T19:49:03Z

I realised I had accidentally introduced an unintended change (related to the discussion for #132) as part of 23a13a5#r94837041. I have since reversed this change: 830b1f0.

Please accept my apologies.

nicoespeon · 2017-01-06T07:15:01Z

Thanks for the changes @cgddrd, all seems good to me.

I'll release all of these today 👍

cgddrd added 13 commits December 24, 2016 15:43

Fixed error: 'undefined is not an object (evaluating branchParentComm…

00c8f58

…it.sha1)'.

Added support for landscape labels (0 degree rotation) for horizontal…

634f0a2

… mode, plus fies for tag labels between vertical and horizontal modes.

Added fix to check 'options.template.branch' is defined before attemp…

89d06e6

…ting to acquire its value.

Minor tidy up bits.

6a7a05c

Updated negative Y margin to account for dot size.

4aeb906

Minor formatting updates.

ed3fe96

Added workaround for null path issue with JSHint.

5cc42c9

Added exclusion of VS code auto-generated files.

848cd89

Incremented minor version number to v1.8.0.

29fa546

Created v1.8.0 release via Grunt.

3a6365d

Removed empty if statement from '_drawTextBG'.

d183ac2

Removed empty if statement from '_drawTextBG'.

568bdfc

Merge branch 'orientation-modes-support-label-rotation' of https://gi…

a428602

…thub.com/cgddrd/gitgraph.js into orientation-modes-support-label-rotation

cgddrd closed this Dec 26, 2016

cgddrd added 7 commits December 26, 2016 22:37

Added utility function to check for undefined values in nested object…

41d3f97

… properties.

Refactored to address JSHint warnings.

4b60312

Refactored to address JSHint warnings.

850ae12

Merged additional refactorings to address JSHint warnings.

003526c

Added default values for 'labelRotation' property for 'metro' and 'bl…

6023654

…ackarrow' template definitions.

Refactored '_isDefined' to become 'isNullOrUndefined' (flipped boolea…

00b9463

…n return values for condition checking).

Corrected issue where '_isNullorUndefined' returned false values inco…

bd35377

…rrectly, and addressed JSHint warnings within '.every()' polyfill.

cgddrd reopened this Dec 27, 2016

cgddrd changed the title ~~Orientation modes support label rotation~~ Support branch label rotation in 'horizontal mode'. Dec 27, 2016

nicoespeon suggested changes Jan 2, 2017

View reviewed changes

cgddrd added 2 commits January 2, 2017 20:03

Applied changes requested following review of nicoespeon#134.

23a13a5

Merged changes from upstream develop branch into local repo.

5378d71

Merged changes from nicoespeon/develop into cgddrd/orientation-modes-…

838913d

…support-label-rotation.

Reverted accidental modification conducted as proof of concept under …

830b1f0

…discussion for nicoespeon#132.

cgddrd commented Jan 5, 2017

View reviewed changes

nicoespeon approved these changes Jan 6, 2017

View reviewed changes

nicoespeon merged commit 5a3326d into nicoespeon:develop Jan 6, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support branch label rotation in 'horizontal mode'. #134

Support branch label rotation in 'horizontal mode'. #134

cgddrd commented Dec 26, 2016 •

edited

Loading

cgddrd commented Dec 26, 2016

cgddrd commented Dec 27, 2016

nicoespeon commented Jan 2, 2017

cgddrd commented Jan 2, 2017

nicoespeon left a comment

nicoespeon Jan 2, 2017

nicoespeon Jan 2, 2017

nicoespeon Jan 2, 2017

cgddrd Jan 2, 2017

nicoespeon Jan 2, 2017

nicoespeon Jan 2, 2017

cgddrd Jan 2, 2017

nicoespeon Jan 2, 2017

cgddrd Jan 2, 2017

nicoespeon Jan 2, 2017

cgddrd Jan 2, 2017

nicoespeon Jan 2, 2017

cgddrd Jan 2, 2017

cgddrd commented Jan 5, 2017

cgddrd Jan 5, 2017 •

edited

Loading

cgddrd commented Jan 5, 2017 •

edited

Loading

nicoespeon commented Jan 6, 2017

Support branch label rotation in 'horizontal mode'. #134

Support branch label rotation in 'horizontal mode'. #134

Conversation

cgddrd commented Dec 26, 2016 • edited Loading

cgddrd commented Dec 26, 2016

cgddrd commented Dec 27, 2016

nicoespeon commented Jan 2, 2017

cgddrd commented Jan 2, 2017

nicoespeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgddrd commented Jan 5, 2017

cgddrd Jan 5, 2017 • edited Loading

Choose a reason for hiding this comment

cgddrd commented Jan 5, 2017 • edited Loading

nicoespeon commented Jan 6, 2017

cgddrd commented Dec 26, 2016 •

edited

Loading

cgddrd Jan 5, 2017 •

edited

Loading

cgddrd commented Jan 5, 2017 •

edited

Loading