Revamp of mode bar's createIcon #2762

vdh · 2018-06-28T01:21:44Z

As a fix for #1335, a backwards-compatible change to createIcon in modebar.js.

If an existing icon config is given, it behaves the same as before.
If an icon config defines height, it will be used instead of ascent and descent.
If an icon config defines transform, it will be used instead of the default transform.
If an icon config doesn't define either a transform or an ascent, it will not be transformed at all.
As an extra bonus, if an icon config is a function it will be called instead of createIcon, in the case where someone wants to completely customise the icon element generation (i.e. inline their own SVGs, or another element).

The pull_font_svg.js util was also updated to regenerate a new ploticon.js, containing height and transform props instead of the previous ascent and descent props. Assuming that this icon data is only used for its intended purpose in the mode bar (i.e. not used/manipulated manually for anything else by users), this shouldn't cause any breakages.

etpinard

Very nice PR. Thanks very much!

We'll need to add a few tests in modebar_test.js before merging, let me know if you need help.

etpinard · 2018-06-28T14:26:01Z

src/components/modebar/modebar.js

-    button.appendChild(this.createIcon(config.icon || Icons.question, config.name));
+    var icon = config.icon;
+    if(typeof icon === 'function') {
+        button.appendChild(icon());


Why would config.icon ever be a function?

To make it easier to manually create the SVG element and bypass createIcon altogether.

For example, I have a version of this patched into my own code, and I use the icon function to generate an SVG with two g group elements with two icon path elements, with the second one set to opacity: 0. I can then use CSS based around modebar-btn and the active class to toggle between the two icon paths when Plotly toggles the active state of the button.

We like to keep our API JSON-serializable, unless we have to (like modebar button click handlers), so I'd vote 👎

The way you implemented the use case you described seems a little hacky. Maybe instead we could add a field named e.g activeIcon that would be used when active class is turned on?

@etpinard
This issue arose because the current custom icon support is extremely tailored and twisted to only support the structure that the Plotly icons have been stored in. I mean, you've got:

Forced transformation inversion of the entire image… just because…?

Limited to only one path element

The viewBox is pieced together from disparate config variables

ascent and descent is never documented anywhere, leaving poor users to guess at (or poke at source code) to even understand how this will change the resulting SVG.

The fact that the Javascript is manually building the SVGs piece by piece in the first place, instead of loading the SVGs in directly, or applying them via CSS…

A custom transformation being applied to one special icon, but not actually via the icon's own config… no… it's actually applied via the name of the button config that just so happens to use that icon…???

I've tried very hard with this PR to keep everything supported, smooth over some of those wrinkles, while also opening it up enough for sane custom icons… so why is this small function-based escape hatch so bad? Amongst a config already full of existing support for functions?

What if someone wants to use SVG sprite sheets? What if they just want to load in their own SVG source files without jumping through hoops? What if they want to use CSS-based icon sets, to load the images in via their existing asset systems? Or heaven forbid, use more than just one path element?! Arbitrarily forcing your users to be restricted to one little use case for their SVGs just to force a JSON structure is maddening! It's not even like I'm forcing this option either, the JSON structure option still works just fine!

I'm sorry to get so heated about this, but calling it "a little hacky" is pot calling the kettle black 😢

Haha @vdh you raise some good points. FWIW most of the quirks you mention above are due to the structure of SVG fonts - for example https://stackoverflow.com/questions/14203365/why-svg-font-is-mirrored - because that's where we do our own design in the first place, and at least at one point we were also using these icons as a font, though I'm not sure we're still doing that anywhere... not that this justifies an obtuse API, just to explain how we got here.

You're right that config items don't have a hard requirement for JSON (unlike data and layout where we cannot allow functions, for portability). @etpinard has good reason to encourage JSON, as it makes cross-language integrations (Python, R, etc) much cleaner. And, pots and kettles notwithstanding, new API gets held to a higher standard than old locked-in API, #420 is bloated enough already 😉

But I have to say I can see the appeal of a more flexible arrangement, and I don't really see a good alternative here. Multiple paths is a good example, what if you want regions with different colors or something? The use cases you've mentioned could be covered by a string of raw HTML, but I don't think that's better, as it could open an XSS vector. So yeah, I think in this case a function may be our best option.

@vdh sorry, I didn't mean to criticize your implementation by calling it hacky. Finding the right hack can be the smartest thing one can do in some situations. But we can't expect every plotly.js user to be come up with that hack. I was simply wondering if there was another (I won't call it less hacky, say more user-intermediate-user-friendly) solution to your use case.

Multiple paths is a good example, what if you want regions with different colors or something? The use cases you've mentioned could be covered by a string of raw HTML, but I don't think that's better, as it could open an XSS vector. So yeah, I think in this case a function may be our best option.

I can get behind this (at least as a solution for v1).

👌 for typeof icon === 'function'.

etpinard · 2018-06-28T14:27:43Z

src/components/modebar/modebar.js

@@ -169,7 +175,9 @@ proto.createButton = function(config) {
 * @Return {HTMLelement}
 */
 proto.createIcon = function(thisIcon, name) {
-    var iconHeight = thisIcon.ascent - thisIcon.descent,
+    var iconHeight = thisIcon.height !== undefined ?


Let's be more strict here:

isNumeric(this.height) ? Number(thisIcon.height) : thisIcon.ascent - this.decent

#2762 (comment)

vdh · 2018-06-29T05:17:06Z

@etpinard I've added some tests, how do they look?

etpinard · 2018-06-29T14:02:16Z

I've added some tests, how do they look?

Your tests look amazing. Thanks very much!

alexcjohnson · 2018-07-02T11:30:31Z

src/components/modebar/modebar.js

+        // Legacy icon transform calculation
+        var transform = name === 'toggleSpikelines' ?
+            'matrix(1.5 0 0 -1.5 0 ' + thisIcon.ascent + ')' :
+            'matrix(1 0 0 -1 0 ' + thisIcon.ascent + ')';


Thanks for moving pull_font_svg to the new transform property - it's definitely cleaner to calculate that upfront - and for maintaining the legacy version to support any users who did go to the trouble of figuring out ascent and descent. But I don't think we need to carry the toggleSpikelines hack over here - lets take it out, then we don't even need name in createIcon 🎉

now that this is baked into pull_font_svg

alexcjohnson · 2018-07-05T09:57:23Z

@etpinard @vdh I removed the legacy toggleSpikeLines hack as I mentioned #2762 (comment) -> 586ff25
If you're OK with this, I think we're ready to go! 💃

etpinard · 2018-07-05T13:06:32Z

Thanks @alexcjohnson ! Merging this thing 💃

vdh · 2018-07-06T00:26:35Z

@etpinard @alexcjohnson
One possible solution I could think of that would work well enough to negate most needs for the custom function would be to add options to:

open up the API more for CSS-based solutions, so that custom SVGs (or images) can be loaded in externally via CSS e.g. icon: { id: 'custom' } or icon: { className: 'fas fa-stroopwafel' } will create an empty span with the given id/class, or will add id/class onto the generated SVG if the usual path/width/etc options are also provided.
a config option for SVG sprites, like icon: { useHref: '#custom' } which would generate <svg><use href="#custom" /></svg>

palimondo · 2018-08-19T17:37:35Z

Heh, this broke my code temporarily, but the fix was just to replace my own iconHeight computation from ascent and descent with height and using the precomputed transform instead of building one myself from ascent… so I just wanted to say kudos! This is much saner serialization in the json alone (I haven't looked at the rest of changes...).

vdh added 3 commits June 28, 2018 10:58

revamp the mode bar's createIcon to better handle custom icons

f0b99ee

updated ploticon data to store the transform matrix with the icons

789ce44

updated condition to account for an icon ascent of 0

ccba6e9

etpinard reviewed Jun 28, 2018

View reviewed changes

etpinard added feature something new status: in progress labels Jun 28, 2018

vdh added 2 commits June 29, 2018 13:38

stricter numeric check on icon height config

3f45a8f

#2762 (comment)

added tests for mode bar icon creation

52f307e

alexcjohnson reviewed Jul 2, 2018

View reviewed changes

remove legacy toggleSpikelines hack

586ff25

now that this is baked into pull_font_svg

etpinard added status: reviewable and removed status: in progress labels Jul 5, 2018

etpinard added this to the v1.39.0 milestone Jul 5, 2018

etpinard merged commit 07fa382 into plotly:master Jul 5, 2018

etpinard mentioned this pull request Jul 5, 2018

Custom mode bar buttons are displayed upside down #1335

Closed

vdh deleted the create-icon-revamp branch May 30, 2019 22:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revamp of mode bar's createIcon #2762

Revamp of mode bar's createIcon #2762

vdh commented Jun 28, 2018 •

edited

Loading

etpinard left a comment

etpinard Jun 28, 2018

vdh Jun 29, 2018

etpinard Jun 29, 2018

vdh Jul 2, 2018

alexcjohnson Jul 2, 2018

etpinard Jul 3, 2018 •

edited

Loading

etpinard Jul 3, 2018

etpinard Jun 28, 2018

vdh commented Jun 29, 2018

etpinard commented Jun 29, 2018

alexcjohnson Jul 2, 2018

alexcjohnson commented Jul 5, 2018

etpinard commented Jul 5, 2018

vdh commented Jul 6, 2018 •

edited

Loading

palimondo commented Aug 19, 2018

Revamp of mode bar's createIcon #2762

Revamp of mode bar's createIcon #2762

Conversation

vdh commented Jun 28, 2018 • edited Loading

etpinard 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

etpinard Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdh commented Jun 29, 2018

etpinard commented Jun 29, 2018

Choose a reason for hiding this comment

alexcjohnson commented Jul 5, 2018

etpinard commented Jul 5, 2018

vdh commented Jul 6, 2018 • edited Loading

palimondo commented Aug 19, 2018

vdh commented Jun 28, 2018 •

edited

Loading

etpinard Jul 3, 2018 •

edited

Loading

vdh commented Jul 6, 2018 •

edited

Loading