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

network - glyph - node font.multi fontAwesome icon blank #3408

Closed
dockstreet opened this issue Sep 2, 2017 · 25 comments
Closed

network - glyph - node font.multi fontAwesome icon blank #3408

dockstreet opened this issue Sep 2, 2017 · 25 comments

Comments

@dockstreet
Copy link
Contributor

dockstreet commented Sep 2, 2017

Hi,

I tried to specify a node from the multifont example with a FontAwesome glyph, but the icon I was trying to use showed up blank?

{ id: 3, font: { multi: 'html', face: 'FontAwesome' }, label: ' \uf286 <b>This</b> is an\n<i>html</i> <b><i>multi-</i>font</b> <code>label</code>', x: 40, y: 40 },

Is it expected or a bug?

Or perhaps there is another way to combine an image with text in an node without SVG? kind regards!!!

@dockstreet dockstreet changed the title network - glyph - node font.multi fontAwesome network - glyph - node font.multi fontAwesome icon blank Sep 2, 2017
@mbroad
Copy link

mbroad commented Sep 4, 2017

When you resize or interact with the graph in some way, does the icon begin to appear?

I'm wondering if this is related to #1835

@dockstreet
Copy link
Contributor Author

dockstreet commented Sep 5, 2017

Hmm.... I did try to resize, but still same result, perhaps it is related? thanks for looking into this!

image

@wimrijnders
Copy link
Contributor

wimrijnders commented Sep 5, 2017

Frankly, I'm amazed that you got that far. You might be the first to try it.
I don't think this is implemented, but I'll have a look.

Update: OK, weird. There is no reason why it shouldn't work, at least I can't find it. Will get back to this.

@sawatts
Copy link

sawatts commented Sep 5, 2017

Coincidentally I have just been trying to do this myself - setting the italic and bold italic faces to fontawesome so that I can add icons in two different colours to the label along with regular text.

Didn't work for me either.

@wimrijnders
Copy link
Contributor

wimrijnders commented Sep 5, 2017

Er. Okay. Uhm. I got it.

download

Eventually, all I did was upgrade FontAwesome from v4.3.0 to v4.5.0:

  <link rel="stylesheet" href="http://maxcdn.bootstrapcdn.com/font-awesome/4.5.0/css/font-awesome.min.css">

.... because I accidentally noticed that some icons were working.

Go ahead, facepalm. It is that kind of moment. I did.

@wimrijnders
Copy link
Contributor

On the other hand, I am somewhat excited that this works at all! I didn't know you could throw around icons in label texts and have it working right out of the box. Really cool.

@sawatts
Copy link

sawatts commented Sep 5, 2017

Not seeing FontAwesome glyphs in multi labels for FA v4.7.0. They are fine as node icons and in regular HTML constructs.

I had the nodes (option default) font.ital.face to FontAwesome, font.ital.mod to empty string, font.multi to true. The node-specific label then contains <i>\uf0c2</i> some other text... for a cloud glyph (amongst others).

Can't say I've used multi-font labels before though - so something may be wrong. However the "blank boxes" were the correct colour at least.

@wimrijnders
Copy link
Contributor

FA 4.7 working for me:

download 1

But I'm not doing anything as fancy as changing multi-fonts. Will get back to it later.

@sawatts
Copy link

sawatts commented Sep 5, 2017

This may be back to loading the options after the network has been formed (cf. #3350). A couple of the nodes are refreshed from the server and their label icons then appear (correct colour, way too big).

I'll try to refactor to download the options before the network is first created.

@sawatts
Copy link

sawatts commented Sep 5, 2017

Nope. Only creating the network after I've pulled the options from the server doesn't change behaviour (but at least the edges are colours after the destination!).

When the graph is first drawn, all glyphs in the labels are drawn as missing characters (cf []) but have the correct colour. On the first update from the server, the Icon based nodes now show the glyphs in the label - but these are the same size as the Icon itself (changing the font.ital size to 4 hasn't changed the label glyph size). Other nodes (all shape nodes) are unchanged.

FontAwesome icons are also used in the general display (buttons etc).

I probably have an overly complex set up here to try to pin down the issue.

@dockstreet
Copy link
Contributor Author

Would it be challenging to also include an image such as a plot or graph into the node as well? not sure if the canvas supports to include an image besides the glyphs?

@wimrijnders
Copy link
Contributor

wimrijnders commented Sep 6, 2017

@dockstreet yes, it would be challenging. Icons work here because they get treated as characters and they just go along with the flow.

Images are supported, but only by themselves, e.g. see example imagesWithBorders.

A trick to combine text and images is to use HTML overlays, meaning that you have a floating div on top of a node. See e.g. this demo from #3300. This is a form of cheating in the sense that these are not true nodes within the network; but if it works for you, hey, who cares.

@sawatts
Copy link

sawatts commented Sep 7, 2017

Okay I narrowed down the issue(s) I was having, and managed to get good FontAwesome characters in labels (with exceptions).

  • Setting 'FontAwesome' as the face for a sub-font (ital, boldital, bold) doesn't work - the glyphs appear as unknown '[]'.
  • Setting 'FontAwesome' as the main 'font.face' and using the sub-fonts to assign colour (and empty mod) allows coloured glyphs in the labels of shapes; but regular characters are in serif rather than sanserif/arial.

But...

  • In both cases (and as described above) FontAwesome glyphs in the labels of icon nodes (where the icon is from FontAwesome) appear with the same size as the node icon.

In my model the node shapes are set through their node group, but colours defined per node. I haven't tried with image-based nodes as I don't have any in the model I am using.

It is cool that this (nearly) works out of the box.

Update: Noticed that the icon-nodes initially draw the label-glyphs at the correct size, but in the wrong colour (default text color). When the topology updates they get the correct colour but the wrong size.

@wimrijnders
Copy link
Contributor

@sawatts That's great sleuth-work! Can I entice you to make a demo for this, on jsfiddle or suchlike?

@sawatts
Copy link

sawatts commented Sep 7, 2017

Okay first example with various nodes and labels in jsbin of course works fine. This shows examples with the node's main font set to FA, and some with just italic and bold fonts set to FA.

The only error I see is when I update one of the nodes and the glyphs in the label are lost - uncomment the 'nodes.update()' at the end to effect this.

One difference between this and my real-case is that the real case uses groups to set all but the colors and label content, while the example does it all within individual nodes. I will try produce a copy which moves properties to groups instead...

@sawatts
Copy link

sawatts commented Sep 7, 2017

New jbin example of the same graph with font and icon properties moved to groups, and just colours overridden in the individual node.

This now shows "multi-font" not working in labels when the fonts are set through the groups rather than in the node (bad) - for both shape and icon nodes.

Still don't see any examples of the icon-sized glyphs though. What else is different...

@sawatts
Copy link

sawatts commented Sep 7, 2017

Big characters are here! jbin.

The last difference was that I set the font properties for all 'nodes', the groups just set the icon to use, and the individual nodes set the colors and labels. With this arrangement we see the icon-sized characters in the labels of icon nodes.

This reverts all nodes to a single font (not multi-font). Switching to multi-font for all nodes we see the following jsbin. None of the label-glyphs display, except in the labels of icon nodes, where they are icon-sized.

One different still is that in the "real" case both glyphs appear large in the icon labels, whereas here just the first one is. Possible caching issue - but it doesn't change in the example if I change the glyph used in the title.

@wimrijnders
Copy link
Contributor

@sawatts Excellent work! I hereby promise to honor it with a serious investigation of the code (aka 'WTF is happening').

I wish I had some token currency to reward you with. All I have is this beer icon 🍺 which I hope to share with you 🍻 .

@wimrijnders
Copy link
Contributor

wimrijnders commented Sep 9, 2017

Looking at the code. Heavy going, but I was expecting that.

I'll be updating this list as I go - think of it as observations during debugging. Putting them here because I want them in a central location. The added benefit for you is that you can see something is happening.


Observations

  • (Multi)font options present on node as well as label.
    • Node-specific options not set in Node instance on call to ctor!
    • They are set in Label instance (this.labelModule.fontOptions).
  • call to network.update():
    • this.labelModule.fontOptions reset to defaults of label options in Node.setOption(), call this.updateLabelModule().
    • After this, defaults are used.

Order of definition for mod-font

It should be (bold used as example):

    options.font.bold -> options.font -> Default.bold -> Default.font

It actually is:

    font.bold -> Default.bold

This is also the reason why call util.fillIfDefined() in Label.parseOptions() fails. Method relies on properties to copy to be present in target object.

This should be fixed either through nifty prototype chaining (hard) or by a specific method call.

Notes

util.selectiveNoDeepExtend:

  • param props is list of first-level options not to copy. commenting adjusted.

Removing a value in options:

  • To delete, set option value to explicit null. Needs param set: allowDeletion = true. This param is internal
  • Usage of allowDeletion is consistent in the extend-routines, but not fully propagated

TODO:

  • Test if setting null in DataSet.update() and Network.setOptions() works as expected.
  • Checks docs for usage null for deletion, adjust if necessary.

@wimrijnders
Copy link
Contributor

wimrijnders commented Sep 14, 2017

Hi guys, this is to inform you that I've managed to fix the bug in the first example. It was a combination of bad transferral of options and, well, a really indecent, banal bug. If you must know, here is the fixed version:

    if (fontOptions[mod][option] !== undefined) {  // Grumbl leaving out test on undefined equals false for ""
...
    };

The amount of time to find that was preposterous. I don't mean to whine (and bragging is out of the question), but this was a really, really hard thing to fix. IMHO of course. But there's a lesson here.

With any luck this also takes care of the issues in the rest of the examples. Will check at a later time.

@wimrijnders
Copy link
Contributor

wimrijnders commented Sep 14, 2017

FYI second example works perfectly now.
Third example (big icons in label text) also fixed.
Aaaand the fourth example.....
download 6
...not quite, 2 icons missing. But it's getting there.

@wimrijnders
Copy link
Contributor

Guys, thanks for this issue. It uncovered quite a bit of manure in the label-handling code. This is slightly offset by the amount of work it turned out to be, but I'll survive.

@dockstreet
Copy link
Contributor Author

@wimrijnders - thanks so much for the hard work !!!!

@wimrijnders
Copy link
Contributor

wimrijnders commented Sep 15, 2017

After examination, I conclude that the two missing icons are, in fact, correctly displayed.
Well, not really 'correct', but it's according to the set options.

The relevant nodes are:

    {
      id: 3, 
      label: '\uf286 glyph shape',
      group: 'glyphs'
    },
    {
      id: 4, 
      label: '\uf286 glyph icon',
      group: 'glyphs',
      shape : 'icon',
      icon : { color: 'green' }
    },

Note that the icons in the label text do not have tags.

The relevant options are:

  var options = 
  {
...      
    nodes :
    {
      font :
      {
        multi : true,
        ital : { face : 'FontAwesome',mod : '' },
        bold : { face : 'FontAwesome',mod : '' }
      }
    }
  };

Note that FontAwesome is used for the tags only. Non-tagged text will therefore use the default font, which is arial, which does not contain icons.


Which means I'm done here. I'm looking at one more thing derived from this issue, which is dynamically changing font information in the groups. After that, I can submit the PR.

wimrijnders added a commit to wimrijnders/vis that referenced this issue Sep 26, 2017
wimrijnders added a commit to wimrijnders/vis that referenced this issue Sep 26, 2017
yotamberk pushed a commit that referenced this issue Sep 29, 2017
* The next fix on Travis unit test failure

This is the next escalation on the war against the Travis unit tests failing (which came into being by yours truly).
By accident, I could recreate the unit test failure on my development machine. This led to a more directed effort to
squash the bug.

The insight here is that test `(window === undefined)` fails, but `(typeof window === 'undefined`)` succeeds. This undoubtedly has to do with the special status `window` has as a global object.

Changes:

- Added check on presence of `window` in `Canvas._requestNextFrame()`, fixed local source errors.
- Added catch clause in `CanvasRendered._determinePixelRatio()`
- small fix: raised timeout for the network `worldCup2014` unit test

* Preliminary refactoring in utils.js

* Added unit tests for extend routines, commenting and small fixes

* More unit tests for extend routines

* - Completed unit tests for extend routines in
- Small fixes and cleanup in `util.js`
- Removed `util.protoExtend()`, not used anywhere

* Added unit tests for known font options

* Interim save before trying out another proto chain strategy

* Fixed problem in first example #3408

* Removed silly file that shouldn't be there

* Added unit test for multi-fonts

* Comment edits

* Verufy unit tests, small adjustments for groups

* Further work on getting unit tests to work. PARTS NEED TO BE CLEANED UP!

* Further tweaks to get unit tests working

* All unit tests passing

* Fixes due to linting

* Small edits

* Removed prototype handling from font pile

* Fixes during testing examples of #3408

* Added unit test for edge labels, small fixes

* Added unit tests for shorthand string fonts; some tests still failing

* All unit tests pass

* Removed Label.parseOptions()

* Completed shorthand font tests, code cleanup, fixed choosify for edges

* Addressed review comments

* Addressed review comments, cleanup
wimrijnders added a commit to wimrijnders/vis that referenced this issue Oct 14, 2017
Bumped up the version number in the examples using `FontAwesome`, in order to avoid confusion about missing (new) icons as in almende#3408.
yotamberk pushed a commit that referenced this issue Oct 14, 2017
Bumped up the version number in the examples using `FontAwesome`, in order to avoid confusion about missing (new) icons as in #3408.
@sawatts
Copy link

sawatts commented Jan 29, 2018

@wimrijnders Many thanks for working this out -- an end-user requirement puts this back in out pot.

primozs pushed a commit to primozs/vis that referenced this issue Jan 3, 2019
* The next fix on Travis unit test failure

This is the next escalation on the war against the Travis unit tests failing (which came into being by yours truly).
By accident, I could recreate the unit test failure on my development machine. This led to a more directed effort to
squash the bug.

The insight here is that test `(window === undefined)` fails, but `(typeof window === 'undefined`)` succeeds. This undoubtedly has to do with the special status `window` has as a global object.

Changes:

- Added check on presence of `window` in `Canvas._requestNextFrame()`, fixed local source errors.
- Added catch clause in `CanvasRendered._determinePixelRatio()`
- small fix: raised timeout for the network `worldCup2014` unit test

* Preliminary refactoring in utils.js

* Added unit tests for extend routines, commenting and small fixes

* More unit tests for extend routines

* - Completed unit tests for extend routines in
- Small fixes and cleanup in `util.js`
- Removed `util.protoExtend()`, not used anywhere

* Added unit tests for known font options

* Interim save before trying out another proto chain strategy

* Fixed problem in first example almende#3408

* Removed silly file that shouldn't be there

* Added unit test for multi-fonts

* Comment edits

* Verufy unit tests, small adjustments for groups

* Further work on getting unit tests to work. PARTS NEED TO BE CLEANED UP!

* Further tweaks to get unit tests working

* All unit tests passing

* Fixes due to linting

* Small edits

* Removed prototype handling from font pile

* Fixes during testing examples of almende#3408

* Added unit test for edge labels, small fixes

* Added unit tests for shorthand string fonts; some tests still failing

* All unit tests pass

* Removed Label.parseOptions()

* Completed shorthand font tests, code cleanup, fixed choosify for edges

* Addressed review comments

* Addressed review comments, cleanup
mojoaxel pushed a commit to visjs/vis_original that referenced this issue Jun 9, 2019
)

Bumped up the version number in the examples using `FontAwesome`, in order to avoid confusion about missing (new) icons as in almende#3408.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants