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

Changes from 1 commit
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
29 changes: 22 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 @@ -155,7 +155,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 +175,9 @@ proto.createButton = function(config) {
* @Return {HTMLelement}
*/
proto.createIcon = function(thisIcon, name) {
var iconHeight = thisIcon.ascent - thisIcon.descent,
var iconHeight = thisIcon.height !== undefined ?
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's be more strict here:

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

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 +186,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) {
// 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