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

Provide a means to left-align labels in boxes #1265

Merged
merged 10 commits into from
Mar 7, 2016

Conversation

ckane
Copy link
Contributor

@ckane ckane commented Sep 3, 2015

In the Network graph layout, Node labels can have multiple lines, and when using large boxes with lots of multiline data (such as from source code or data dumps) it makes it more readable to left-align the text.

This PR will create a new property called 'textAlign' that can be used on 'box' node shapes to left-align, rather than center-align the text label within the box.

Will only apply to Network nodes with a shape: 'box' property, as most
other shapes this will not make sense for. This enables the box shape
to be used, for instance, for diagraming CFG data from source code.
@AlexDM0
Copy link
Contributor

AlexDM0 commented Sep 3, 2015

Hi,

Thanks for wanting to contribute! I'm sorry but I can't accept a feature that only works for one type of node.

Do you have an example for why this would be better in general? I don't see why this alignment would be better for anything else than a specific use case.

I want to enable the alignment option that currently exists for edges to also work for nodes, though I did not want to align it inside the node anywhere else than center.

Regards

On 3 sep. 2015, at 16:38, Coleman Kane [email protected] wrote:

In the Network graph layout, Node labels can have multiple lines, and when using large boxes with lots of multiline data (such as from source code or data dumps) it makes it more readable to left-align the text.

This PR will create a new property called 'textAlign' that can be used on 'box' node shapes to left-align, rather than center-align the text label within the box.

You can view, comment on, or merge this pull request online at:

#1265

Commit Summary

Merge pull request #1 from almende/develop
Merge pull request #2 from ckane/develop
Allow node labels to be left-justified with textAlign: 'left'
File Changes

M lib/network/modules/NodesHandler.js (3)
M lib/network/modules/components/shared/Label.js (8)
Patch Links:

https://github.com/almende/vis/pull/1265.patch
https://github.com/almende/vis/pull/1265.diff

Reply to this email directly or view it on GitHub.

@ckane
Copy link
Contributor Author

ckane commented Sep 3, 2015

IMO - The reason why it applies to this specific type of node is because there are use cases where this particular type of node is the only one (I think) that would be suitable for diagramming blocks of data such as blocks of code.

I don't think most of the other shape types, other than "text" and maybe "database" would be appropriate for this type of alignment. However, any of the shapes that display their label outside the graphic might be good candidates for this. I could just remove the box-specific tests and it could be the user's responsibility to make it not look ugly.

The big thing here is I want multi-line data rendered within the label, but I don't want it to center everything, because sometimes that's not visually appealing. I also want to be able to see the content of multiple nodes within my diagram, without having to click each one individually and use JS to fill another panel of the browser window with details.

Inserting screenshot of what I have so far.

screen shot 2015-09-03 at 2 36 46 pm

Theoretically, there are multiple node shape types that this could apply to.
@ckane
Copy link
Contributor Author

ckane commented Sep 5, 2015

I changed the code to not rely upon "box".

Theoretically, this could also be useful when laying out multi-record data in visual format using the following shapes:

  • star
  • text
  • triangle
  • triangleDown
  • diamond
  • database
  • square
  • dot

@@ -133,7 +133,8 @@
background: 'none',
strokeWidth: 0, // px
strokeColor: '#ffffff',
align: 'horizontal'
align: 'horizontal',
textAlign: 'center'
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we just use the align for this? Align is currently used by edges only. We could use it for your case when we're using nodes.

@AlexDM0 AlexDM0 added the Network label Sep 7, 2015
@ckane
Copy link
Contributor Author

ckane commented Sep 7, 2015

I'll make a change to just pull from "align" as you said, rather than textAlign. I didn't realize the "align" property was ignored for nodes.

@AlexDM0
Copy link
Contributor

AlexDM0 commented Sep 7, 2015

Could you also make an example showing the functionality? The image you posted here looks like a new usecase so it might be interesting for people looking to try vis.

@ckane
Copy link
Contributor Author

ckane commented Sep 7, 2015

So I'm looking through the code and it seems like drawBackground() and setAlignment() in Label.js expect the fontOptions.align to be describing vertical alignment (middle, top, bottom, horizontal?)... not sure if negative impact from clobbering that.

@AlexDM0
Copy link
Contributor

AlexDM0 commented Sep 7, 2015

You'll have to safeguard that yes :) Maybe pass the type of the object into the constructor so it knows edge or node?

Its ugly, yes but I think its the easiest way to do this.

Cheers

@ckane
Copy link
Contributor Author

ckane commented Sep 7, 2015

Ah yes, good point - I forgot Label.js is used for both edge labels & node labels. So the default of "horizontal" in NodesHandler.js is irrelevant then...

@ckane
Copy link
Contributor Author

ckane commented Sep 7, 2015

Oh nice - this helps resolve a TODO at line 157

Node labels don't currently make use of the 'align' property, so this reuses
the existing property.

The label code has a number of alignment options that make sense to edges only,
so the Edge/Node now passes a boolean to the Label ctor to explain which the label
is being rendered for
@ckane
Copy link
Contributor Author

ckane commented Sep 7, 2015

BTW: yeah I can whip up a nifty disasm control-flow-graph example when the details get settled. The above was generated by automation we have in our malware analysis program.

@ckane
Copy link
Contributor Author

ckane commented Sep 7, 2015

Ok - I created a diff and patched against 4.8.1 and the effect works well, and there don't seem to be regressions in the standard label layout now that the default is set to 'center' for nodes' font.align property. I tested some of my graphs of disassembly, as well as some other graphs I have that use the existing behavior too.

@ckane
Copy link
Contributor Author

ckane commented Sep 30, 2015

Sorry for the delay on this - i got derailed ... still planning on putting together examples for you

@AlexDM0
Copy link
Contributor

AlexDM0 commented Nov 3, 2015

Hi,

Hows this coming?

Cheers

@ckane
Copy link
Contributor Author

ckane commented Dec 12, 2015

Just got done with finals week. $dayjob should provide some time for me to get this updated. If I am not mistaken, you just want me to add an example to the documentation? Thanks for the pings.

@ckane
Copy link
Contributor Author

ckane commented Mar 6, 2016

Ok @AlexDM0 I finally had time this weekend to add to the examples. I updated the existing "labels" example, and I also added a new "example application" named disassemblerExample.html that gives a sample of the application I was requesting the feature change for.

Sorry it took so long. Life is hard.

@ckane
Copy link
Contributor Author

ckane commented Mar 6, 2016

BTW network_examples.html doesn't appear to be in the repo, so I wasn't able to edit that file for your docs.

@AlexDM0
Copy link
Contributor

AlexDM0 commented Mar 7, 2016

Looks good! Thanks!

AlexDM0 added a commit that referenced this pull request Mar 7, 2016
Provide a means to left-align labels in boxes
@AlexDM0 AlexDM0 merged commit 1724c1b into almende:develop Mar 7, 2016
@AlexDM0
Copy link
Contributor

AlexDM0 commented Mar 7, 2016

For completeness, should we include align-right and justify as well?

@ckane
Copy link
Contributor Author

ckane commented Mar 7, 2016

Yeah I could probably test that out too and verify it works easily enough, or determine if either one of them cause unforeseen visual glitches.

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.

2 participants