Skip to content

Revamp of mode bar's createIcon #2762

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jul 5, 2018
Merged

Revamp of mode bar's createIcon #2762

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f0b99ee
revamp the mode bar's createIcon to better handle custom icons
vdh Jun 28, 2018
789ce44
updated ploticon data to store the transform matrix with the icons
vdh Jun 28, 2018
ccba6e9
updated condition to account for an icon ascent of 0
vdh Jun 28, 2018
3f45a8f
stricter numeric check on icon height config
vdh Jun 29, 2018
52f307e
added tests for mode bar icon creation
vdh Jun 29, 2018
586ff25
remove legacy toggleSpikelines hack
alexcjohnson Jul 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 40 additions & 40 deletions build/ploticon.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,122 +3,122 @@
module.exports = {
'undo': {
'width': 857.1,
'height': 1000,
'path': 'm857 350q0-87-34-166t-91-137-137-92-166-34q-96 0-183 41t-147 114q-4 6-4 13t5 11l76 77q6 5 14 5 9-1 13-7 41-53 100-82t126-29q58 0 110 23t92 61 61 91 22 111-22 111-61 91-92 61-110 23q-55 0-105-20t-90-57l77-77q17-16 8-38-10-23-33-23h-250q-15 0-25 11t-11 25v250q0 24 22 33 22 10 39-8l72-72q60 57 137 88t159 31q87 0 166-34t137-92 91-137 34-166z',
'ascent': 850,
'descent': -150
'transform': 'matrix(1 0 0 -1 0 850)'
},
'home': {
'width': 928.6,
'height': 1000,
'path': 'm786 296v-267q0-15-11-26t-25-10h-214v214h-143v-214h-214q-15 0-25 10t-11 26v267q0 1 0 2t0 2l321 264 321-264q1-1 1-4z m124 39l-34-41q-5-5-12-6h-2q-7 0-12 3l-386 322-386-322q-7-4-13-4-7 2-12 7l-35 41q-4 5-3 13t6 12l401 334q18 15 42 15t43-15l136-114v109q0 8 5 13t13 5h107q8 0 13-5t5-13v-227l122-102q5-5 6-12t-4-13z',
'ascent': 850,
'descent': -150
'transform': 'matrix(1 0 0 -1 0 850)'
},
'camera-retro': {
'width': 1000,
'height': 1000,
'path': 'm518 386q0 8-5 13t-13 5q-37 0-63-27t-26-63q0-8 5-13t13-5 12 5 5 13q0 23 16 38t38 16q8 0 13 5t5 13z m125-73q0-59-42-101t-101-42-101 42-42 101 42 101 101 42 101-42 42-101z m-572-320h858v71h-858v-71z m643 320q0 89-62 152t-152 62-151-62-63-152 63-151 151-63 152 63 62 151z m-571 358h214v72h-214v-72z m-72-107h858v143h-462l-36-71h-360v-72z m929 143v-714q0-30-21-51t-50-21h-858q-29 0-50 21t-21 51v714q0 30 21 51t50 21h858q29 0 50-21t21-51z',
'ascent': 850,
'descent': -150
'transform': 'matrix(1 0 0 -1 0 850)'
},
'zoombox': {
'width': 1000,
'height': 1000,
'path': 'm1000-25l-250 251c40 63 63 138 63 218 0 224-182 406-407 406-224 0-406-182-406-406s183-406 407-406c80 0 155 22 218 62l250-250 125 125z m-812 250l0 438 437 0 0-438-437 0z m62 375l313 0 0-312-313 0 0 312z',
'ascent': 850,
'descent': -150
'transform': 'matrix(1 0 0 -1 0 850)'
},
'pan': {
'width': 1000,
'height': 1000,
'path': 'm1000 350l-187 188 0-125-250 0 0 250 125 0-188 187-187-187 125 0 0-250-250 0 0 125-188-188 186-187 0 125 252 0 0-250-125 0 187-188 188 188-125 0 0 250 250 0 0-126 187 188z',
'ascent': 850,
'descent': -150
'transform': 'matrix(1 0 0 -1 0 850)'
},
'zoom_plus': {
'width': 1000,
'height': 1000,
'path': 'm1 787l0-875 875 0 0 875-875 0z m687-500l-187 0 0-187-125 0 0 187-188 0 0 125 188 0 0 187 125 0 0-187 187 0 0-125z',
'ascent': 850,
'descent': -150
'transform': 'matrix(1 0 0 -1 0 850)'
},
'zoom_minus': {
'width': 1000,
'height': 1000,
'path': 'm0 788l0-876 875 0 0 876-875 0z m688-500l-500 0 0 125 500 0 0-125z',
'ascent': 850,
'descent': -150
'transform': 'matrix(1 0 0 -1 0 850)'
},
'autoscale': {
'width': 1000,
'height': 1000,
'path': 'm250 850l-187 0-63 0 0-62 0-188 63 0 0 188 187 0 0 62z m688 0l-188 0 0-62 188 0 0-188 62 0 0 188 0 62-62 0z m-875-938l0 188-63 0 0-188 0-62 63 0 187 0 0 62-187 0z m875 188l0-188-188 0 0-62 188 0 62 0 0 62 0 188-62 0z m-125 188l-1 0-93-94-156 156 156 156 92-93 2 0 0 250-250 0 0-2 93-92-156-156-156 156 94 92 0 2-250 0 0-250 0 0 93 93 157-156-157-156-93 94 0 0 0-250 250 0 0 0-94 93 156 157 156-157-93-93 0 0 250 0 0 250z',
'ascent': 850,
'descent': -150
'transform': 'matrix(1 0 0 -1 0 850)'
},
'tooltip_basic': {
'width': 1500,
'height': 1000,
'path': 'm375 725l0 0-375-375 375-374 0-1 1125 0 0 750-1125 0z',
'ascent': 850,
'descent': -150
'transform': 'matrix(1 0 0 -1 0 850)'
},
'tooltip_compare': {
'width': 1125,
'height': 1000,
'path': 'm187 786l0 2-187-188 188-187 0 0 937 0 0 373-938 0z m0-499l0 1-187-188 188-188 0 0 937 0 0 376-938-1z',
'ascent': 850,
'descent': -150
'transform': 'matrix(1 0 0 -1 0 850)'
},
'plotlylogo': {
'width': 1542,
'height': 1000,
'path': 'm0-10h182v-140h-182v140z m228 146h183v-286h-183v286z m225 714h182v-1000h-182v1000z m225-285h182v-715h-182v715z m225 142h183v-857h-183v857z m231-428h182v-429h-182v429z m225-291h183v-138h-183v138z',
'ascent': 850,
'descent': -150
'transform': 'matrix(1 0 0 -1 0 850)'
},
'z-axis': {
'width': 1000,
'height': 1000,
'path': 'm833 5l-17 108v41l-130-65 130-66c0 0 0 38 0 39 0-1 36-14 39-25 4-15-6-22-16-30-15-12-39-16-56-20-90-22-187-23-279-23-261 0-341 34-353 59 3 60 228 110 228 110-140-8-351-35-351-116 0-120 293-142 474-142 155 0 477 22 477 142 0 50-74 79-163 96z m-374 94c-58-5-99-21-99-40 0-24 65-43 144-43 79 0 143 19 143 43 0 19-42 34-98 40v216h87l-132 135-133-135h88v-216z m167 515h-136v1c16 16 31 34 46 52l84 109v54h-230v-71h124v-1c-16-17-28-32-44-51l-89-114v-51h245v72z',
'ascent': 850,
'descent': -150
'transform': 'matrix(1 0 0 -1 0 850)'
},
'3d_rotate': {
'width': 1000,
'height': 1000,
'path': 'm922 660c-5 4-9 7-14 11-359 263-580-31-580-31l-102 28 58-400c0 1 1 1 2 2 118 108 351 249 351 249s-62 27-100 42c88 83 222 183 347 122 16-8 30-17 44-27-2 1-4 2-6 4z m36-329c0 0 64 229-88 296-62 27-124 14-175-11 157-78 225-208 249-266 8-19 11-31 11-31 2 5 6 15 11 32-5-13-8-20-8-20z m-775-239c70-31 117-50 198-32-121 80-199 346-199 346l-96-15-58-12c0 0 55-226 155-287z m603 133l-317-139c0 0 4-4 19-14 7-5 24-15 24-15s-177-147-389 4c235-287 536-112 536-112l31-22 100 299-4-1z m-298-153c6-4 14-9 24-15 0 0-17 10-24 15z',
'ascent': 850,
'descent': -150
'transform': 'matrix(1 0 0 -1 0 850)'
},
'camera': {
'width': 1000,
'height': 1000,
'path': 'm500 450c-83 0-150-67-150-150 0-83 67-150 150-150 83 0 150 67 150 150 0 83-67 150-150 150z m400 150h-120c-16 0-34 13-39 29l-31 93c-6 15-23 28-40 28h-340c-16 0-34-13-39-28l-31-94c-6-15-23-28-40-28h-120c-55 0-100-45-100-100v-450c0-55 45-100 100-100h800c55 0 100 45 100 100v450c0 55-45 100-100 100z m-400-550c-138 0-250 112-250 250 0 138 112 250 250 250 138 0 250-112 250-250 0-138-112-250-250-250z m365 380c-19 0-35 16-35 35 0 19 16 35 35 35 19 0 35-16 35-35 0-19-16-35-35-35z',
'ascent': 850,
'descent': -150
'transform': 'matrix(1 0 0 -1 0 850)'
},
'movie': {
'width': 1000,
'height': 1000,
'path': 'm938 413l-188-125c0 37-17 71-44 94 64 38 107 107 107 187 0 121-98 219-219 219-121 0-219-98-219-219 0-61 25-117 66-156h-115c30 33 49 76 49 125 0 103-84 187-187 187s-188-84-188-187c0-57 26-107 65-141-38-22-65-62-65-109v-250c0-70 56-126 125-126h500c69 0 125 56 125 126l188-126c34 0 62 28 62 63v375c0 35-28 63-62 63z m-750 0c-69 0-125 56-125 125s56 125 125 125 125-56 125-125-56-125-125-125z m406-1c-87 0-157 70-157 157 0 86 70 156 157 156s156-70 156-156-70-157-156-157z',
'ascent': 850,
'descent': -150
'transform': 'matrix(1 0 0 -1 0 850)'
},
'question': {
'width': 857.1,
'height': 1000,
'path': 'm500 82v107q0 8-5 13t-13 5h-107q-8 0-13-5t-5-13v-107q0-8 5-13t13-5h107q8 0 13 5t5 13z m143 375q0 49-31 91t-77 65-95 23q-136 0-207-119-9-14 4-24l74-55q4-4 10-4 9 0 14 7 30 38 48 51 19 14 48 14 27 0 48-15t21-33q0-21-11-34t-38-25q-35-16-65-48t-29-70v-20q0-8 5-13t13-5h107q8 0 13 5t5 13q0 10 12 27t30 28q18 10 28 16t25 19 25 27 16 34 7 45z m214-107q0-117-57-215t-156-156-215-58-216 58-155 156-58 215 58 215 155 156 216 58 215-58 156-156 57-215z',
'ascent': 850,
'descent': -150
'transform': 'matrix(1 0 0 -1 0 850)'
},
'disk': {
'width': 857.1,
'height': 1000,
'path': 'm214-7h429v214h-429v-214z m500 0h72v500q0 8-6 21t-11 20l-157 156q-5 6-19 12t-22 5v-232q0-22-15-38t-38-16h-322q-22 0-37 16t-16 38v232h-72v-714h72v232q0 22 16 38t37 16h465q22 0 38-16t15-38v-232z m-214 518v178q0 8-5 13t-13 5h-107q-7 0-13-5t-5-13v-178q0-8 5-13t13-5h107q7 0 13 5t5 13z m357-18v-518q0-22-15-38t-38-16h-750q-23 0-38 16t-16 38v750q0 22 16 38t38 16h517q23 0 50-12t42-26l156-157q16-15 27-42t11-49z',
'ascent': 850,
'descent': -150
'transform': 'matrix(1 0 0 -1 0 850)'
},
'lasso': {
'width': 1031,
'height': 1000,
'path': 'm1018 538c-36 207-290 336-568 286-277-48-473-256-436-463 10-57 36-108 76-151-13-66 11-137 68-183 34-28 75-41 114-42l-55-70 0 0c-2-1-3-2-4-3-10-14-8-34 5-45 14-11 34-8 45 4 1 1 2 3 2 5l0 0 113 140c16 11 31 24 45 40 4 3 6 7 8 11 48-3 100 0 151 9 278 48 473 255 436 462z m-624-379c-80 14-149 48-197 96 42 42 109 47 156 9 33-26 47-66 41-105z m-187-74c-19 16-33 37-39 60 50-32 109-55 174-68-42-25-95-24-135 8z m360 75c-34-7-69-9-102-8 8 62-16 128-68 170-73 59-175 54-244-5-9 20-16 40-20 61-28 159 121 317 333 354s407-60 434-217c28-159-121-318-333-355z',
'ascent': 850,
'descent': -150
'transform': 'matrix(1 0 0 -1 0 850)'
},
'selectbox': {
'width': 1000,
'height': 1000,
'path': 'm0 850l0-143 143 0 0 143-143 0z m286 0l0-143 143 0 0 143-143 0z m285 0l0-143 143 0 0 143-143 0z m286 0l0-143 143 0 0 143-143 0z m-857-286l0-143 143 0 0 143-143 0z m857 0l0-143 143 0 0 143-143 0z m-857-285l0-143 143 0 0 143-143 0z m857 0l0-143 143 0 0 143-143 0z m-857-286l0-143 143 0 0 143-143 0z m286 0l0-143 143 0 0 143-143 0z m285 0l0-143 143 0 0 143-143 0z m286 0l0-143 143 0 0 143-143 0z',
'ascent': 850,
'descent': -150
'transform': 'matrix(1 0 0 -1 0 850)'
},
'spikeline': {
'width': 1000,
'height': 1000,
'path': 'M512 409c0-57-46-104-103-104-57 0-104 47-104 104 0 57 47 103 104 103 57 0 103-46 103-103z m-327-39l92 0 0 92-92 0z m-185 0l92 0 0 92-92 0z m370-186l92 0 0 93-92 0z m0-184l92 0 0 92-92 0z',
'ascent': 850,
'descent': -150
'transform': 'matrix(1.5 0 0 -1.5 0 850)'
}
};
30 changes: 23 additions & 7 deletions src/components/modebar/modebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
'use strict';

var d3 = require('d3');
var isNumeric = require('fast-isnumeric');

var Lib = require('../../lib');
var Icons = require('../../../build/ploticon');
Expand Down Expand Up @@ -155,7 +156,13 @@ proto.createButton = function(config) {
button.setAttribute('data-toggle', config.toggle || false);
if(config.toggle) d3.select(button).classed('active', true);

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

Choose a reason for hiding this comment

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

Why would config.icon ever be a function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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 😢

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@etpinard etpinard Jul 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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'.

}
else {
button.appendChild(this.createIcon(icon || Icons.question, config.name));
}
button.setAttribute('data-gravity', config.gravity || 'n');

return button;
Expand All @@ -169,7 +176,9 @@ proto.createButton = function(config) {
* @Return {HTMLelement}
*/
proto.createIcon = function(thisIcon, name) {
var iconHeight = thisIcon.ascent - thisIcon.descent,
var iconHeight = isNumeric(thisIcon.height) ?
Number(thisIcon.height) :
thisIcon.ascent - thisIcon.descent,
svgNS = 'http://www.w3.org/2000/svg',
icon = document.createElementNS(svgNS, 'svg'),
path = document.createElementNS(svgNS, 'path');
Expand All @@ -178,12 +187,19 @@ proto.createIcon = function(thisIcon, name) {
icon.setAttribute('width', (thisIcon.width / iconHeight) + 'em');
icon.setAttribute('viewBox', [0, 0, thisIcon.width, iconHeight].join(' '));

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

path.setAttribute('d', thisIcon.path);
path.setAttribute('transform', transform);

if(thisIcon.transform) {
path.setAttribute('transform', thisIcon.transform);
}
else if(thisIcon.ascent !== undefined) {
// 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 + ')';
Copy link
Collaborator

Choose a reason for hiding this comment

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

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 🎉

path.setAttribute('transform', transform);
}

icon.appendChild(path);

return icon;
Expand Down
11 changes: 8 additions & 3 deletions tasks/util/pull_font_svg.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,16 @@ module.exports = function pullFontSVG(data, pathOut) {
chars = {};

font_obj.glyph.forEach(function(glyph) {
chars[glyph.$['glyph-name']] = {
var name = glyph.$['glyph-name'],
transform = name === 'spikeline' ?
'matrix(1.5 0 0 -1.5 0 ' + ascent + ')' :
'matrix(1 0 0 -1 0 ' + ascent + ')';

chars[name] = {
width: Number(glyph.$['horiz-adv-x']) || default_width,
height: ascent - descent,
path: glyph.$.d,
ascent: ascent,
descent: descent
transform: transform,
};
});

Expand Down
Loading