modeBar styling #3068

antoinerg · 2018-10-02T18:28:42Z

Initial attempt at closing #3063

There are several little changes in different places and some may not be satisfactory. This initial implementation will be a good starting point to further discuss how to resolve #3063.

Note that currently, this PR doesn't have the hover effect on buttons. To do this in CSS, we would need to inject a new <style> element onto the page to style the pseudo-class :hover. Alternatively, we could change the fill color in javascript onmouseover. Let me know what you would prefer.

antoinerg · 2018-10-02T18:51:55Z

src/components/modebar/modebar.js

    }

-    icon.appendChild(path);
+    if(thisIcon.svg) {
+        icon.innerHTML = thisIcon.svg;


test-syntax fails because or innerHTML :(

This should be fine. innerHTML only causes problems on SVG nodes.

Looks like the concerns written down in

plotly.js/tasks/test_syntax.js

Lines 106 to 109 in 9772ef6

// These are forbidden in IE *only in SVG* but since

// that's 99% of what we do here, we'll forbid them entirely

// until there's some HTML use case where we need them.

// (not sure what we'd do then, but we'd think of something!)

have become reality.

thisIcon.svg isn't a SVG document but rather its content. icon is the SVG document so IE doesn't like this. Let me fix this :)

Fixed in commit e870e34

I got rid of innerHTML altogether and now use DOMParser in commit 61d1c7b !

antoinerg · 2018-10-02T21:06:08Z

BTW, here's what it looks like with a vertical orientation and black icons:

src/fonts/ploticon/ploticon.svg

antoinerg · 2018-10-03T00:26:01Z

He made "negatives" which are monochrome like the sign outside the Montréal office. . I also don't have a strong opinion so let's wait and find out what @alexcjohnson thinks.

Just a reminder, this PR currently has a major regression: there's no hover and active styles for the buttons. There are different ways to achieve this.

alexcjohnson · 2018-10-03T02:32:11Z

If we think people will really change the logo color, then it would be worth using the monochrome version. But I suspect anyone who cares that much what the modebar looks like is going to drop the plotly logo entirely, so we should optimize the look of the existing logo, which is the current multicolor version - so I'd vote to keep the color.

Do we want to reverse the order of the vertical modebar? We should at least move the logo to the top corner, but I think it might work better - at least for people already used to the horizontal one - to have the whole thing flipped as if we had just rotated the existing modebar around the logo.

antoinerg · 2018-10-03T03:41:58Z

@alexcjohnson I agree we should move the logo to the top.

As for the icons, I'd rather have the + icon be to the left or above the - icon. Which is why I prefer the current order of icons. But this is a matter of preference. BTW, it's quite easy to change the order in the code so we can settle this later once we made up our mind :)

Note that I made the icons be squares in order for them to tightly fit into a rectangle both horizontally and vertically. Prior to that change, when vertical, some icons were wider and it wouldn't be aesthetically pleasing. As a result some icons are now smaller than they used to (in particular the icons related to hover). I hope this is fine.

Horizontal

Vertical

alexcjohnson · 2018-10-03T12:59:15Z

Good call on the ➕ / ➖ order. I suppose we could flip groups but... then it gets confusing if people want to set their own icons. OK, lets leave it as you have it now.

Agreed on making them all square in vertical mode. In horizontal mode I do think it looks a bit better with the original sizes, if it's not too difficult to maintain both behaviors.

etpinard · 2018-10-03T13:54:02Z

Good call on the / order. I suppose we could flip groups bu

I'd vote for making the snapshot button appear at the bottom of the vertical list and the hover buttons at the top under orientation: 'v'.

alexcjohnson · 2018-10-03T14:14:10Z

I'd vote for making the snapshot button appear at the bottom of the vertical list and the hover buttons at the top under orientation: 'v'.

Right, that's what I meant by flipping groups - ie the order of groups gets reversed, but within each group the order is maintained. But @etpinard don't you think this would make adding/removing icons (using modeBarButtons(ToAdd|ToRemove)?) confusing?

etpinard · 2018-10-03T14:23:23Z

don't you think this would make adding/removing icons (using modeBarButtons(ToAdd|ToRemove)?) confusing?

I don't think so, if we document it properly. It might a good excuse to ✅ #2703

antoinerg · 2018-10-03T15:38:33Z

Reversing the order of groups looks like this:

As long as the ➕ is above ➖, I don't have a preference.

etpinard · 2018-10-03T19:42:48Z

src/plots/layout_attributes.js

@@ -224,5 +224,30 @@ module.exports = {
            'or a logo image, for example. To omit one of these items on the plot,',
            'make an item with matching `templateitemname` and `visible: false`.'
        ].join(' ')
+    },
+    modeBarStyle: {


no 🐫 in data/layout attributes!

... but wait do we really want to make these layout attributes?

Ha I see, @alexcjohnson , @nicolaskruchten and @jackparmer have agreed on making these layout attributes in #3063. Not much I can say I guess.

Maybe you could chime in on how we should name those attributes. I didn't realize there was no 🐫 in data/layout attributes! 😳

I'd vote for:

modebar: { orientation: 'h' | 'v', bgcolor: '', color: /* implied icon color */, activecolor: /* just like we currently use for rangeselector */ }

I vote for @etpinard's suggestion. What do you guys think? @alexcjohnson @nicolaskruchten @jackparmer

antoinerg · 2018-10-04T17:51:50Z

Commit b7c2355 updates the CSS to place tooltips on the left of vertical modebars!

etpinard · 2018-10-04T17:43:41Z

src/css/_modebar.scss

+        display: block;
+        float: none;
+        margin-left: 0px;
+        margin-top: 8px;


Looks like the vertical orientation has more top margin than the what the horizontal orientation has to its right.

Is this on purpose? This is just something I noticed, I don't have a strong opinion on what this should look like.

Nice catch 👀 ! No, it's not on purpose. I'd rather have the logo at the top. Fixing this right now!

etpinard · 2018-10-04T17:44:24Z

build/ploticon.js

+    },
+    'newplotlylogo': {
+        'name': 'newplotlylogo',
+        'svg': '<svg xmlns=\'http://www.w3.org/2000/svg\' viewBox=\'0 0 132 132\'><defs><style>&#xD;\n      .cls-1 {&#xD;\n        fill: #119dff;&#xD;\n      }&#xD;\n&#xD;\n      .cls-2 {&#xD;\n        fill: #25fefd;&#xD;\n      }&#xD;\n&#xD;\n      .cls-3 {&#xD;\n        fill: #fff;&#xD;\n      }&#xD;\n    </style></defs><title>plotly-logomark</title><g id=\'symbol\'><rect class=\'cls-1\' width=\'132\' height=\'132\' rx=\'6\' ry=\'6\'/><circle class=\'cls-2\' cx=\'78\' cy=\'54\' r=\'6\'/><circle class=\'cls-2\' cx=\'102\' cy=\'30\' r=\'6\'/><circle class=\'cls-2\' cx=\'78\' cy=\'30\' r=\'6\'/><circle class=\'cls-2\' cx=\'54\' cy=\'30\' r=\'6\'/><circle class=\'cls-2\' cx=\'30\' cy=\'30\' r=\'6\'/><circle class=\'cls-2\' cx=\'30\' cy=\'54\' r=\'6\'/><path class=\'cls-3\' d=\'M30,72a6,6,0,0,0-6,6v24a6,6,0,0,0,12,0V78A6,6,0,0,0,30,72Z\'/><path class=\'cls-3\' d=\'M78,72a6,6,0,0,0-6,6v24a6,6,0,0,0,12,0V78A6,6,0,0,0,78,72Z\'/><path class=\'cls-3\' d=\'M54,48a6,6,0,0,0-6,6v48a6,6,0,0,0,12,0V54A6,6,0,0,0,54,48Z\'/><path class=\'cls-3\' d=\'M102,48a6,6,0,0,0-6,6v48a6,6,0,0,0,12,0V54A6,6,0,0,0,102,48Z\'/></g></svg>'


Could we trim those \n and spaces?

Fixed by commit dc7877b

etpinard · 2018-10-04T17:48:03Z

src/components/modebar/modebar.js

+        buttons = buttons.reverse();
+    }
+
+    Lib.addRelatedStyleRule(modeBarId, '#' + modeBarId, 'background-color: ' + fullLayout.modebar.bgcolor);


Do we have to use CSS here? Could we instead add mouseover / mouseout handlers to the modebar buttons, similar to how we handle updatemenu buttons:

plotly.js/src/components/updatemenus/draw.js

Lines 219 to 221 in 30ed4a4

header.on('mouseover', function() {

header.call(styleOnMouseOver);

});

plotly.js/src/components/updatemenus/draw.js

Lines 464 to 467 in 30ed4a4

function styleOnMouseOver(item) {

item.select('rect.' + constants.itemRectClassName)

.call(Color.fill, constants.hoverColor);

}

that way we wouldn't have to hijack the <style> dynamically.

We could use mouseover / mouseout for hover but I wasn't sure how to handle the active state. Since we already add/remove class name active to the icons, I thought it made sense to just style those in CSS. @alexcjohnson wasn't against the idea but we can consider another strategy.

Since we already add/remove class name active to the icons

Right, getting "active" to work would require some work. The modebar button click handlers would need to update their style attribute instead of appending/removing the "active" class name.

Consider my comment non-blocking.

etpinard · 2018-10-04T17:53:00Z

src/components/modebar/modebar.js

+    Lib.addRelatedStyleRule(modeBarId, '#' + modeBarId, 'background-color: ' + fullLayout.modebar.bgcolor);
+    Lib.addRelatedStyleRule(modeBarId, '#' + modeBarId + ' .modebar-btn .icon path', 'fill: ' + fullLayout.modebar.color);
+    Lib.addRelatedStyleRule(modeBarId, '#' + modeBarId + ' .modebar-btn:hover .icon path', 'fill: ' + fullLayout.modebar.activecolor);
+    Lib.addRelatedStyleRule(modeBarId, '#' + modeBarId + ' .modebar-btn.active .icon path', 'fill: ' + fullLayout.modebar.activecolor);


These don't appear to update properly on relayout e.g. Plotly.relayout(gd, 'modebar.bgcolor', 'blue')

moreover, modebar.color doesn't seem to work on some icons.

Let me investigate relayout!

As for the screen capture: the gray icons are the ones that are currently active (gray being the default activecolor). It does look like a bug the first time you try it but it's not. Maybe I should implement what @nicolaskruchten suggested over here: #3063 (comment). This shouldn't be a problem in Dash however since they usually explicitly style everything.

the gray icons are the ones that are currently active (gray being the default activecolor).

Ha I see. My mistake!

Commit 97f25e2 fixes the issue with relayout.

The new style rules were just appended to the same style element and would have less priority than previous rules. This is fixed by starting with a fresh style element each time.

Cool. Let's 🔒 this down in a jasmine test though.

In commit f5cc0e9, we test each new layout attributes and make sure they behave properly when using relayout!

Great tests. Thanks 🎉

antoinerg · 2018-10-05T18:22:55Z

Ok see here's how the default vertical modebars look like with the latest commit ff3f9f6:

What do you guys think? Should I choose better defaults?

alexcjohnson · 2018-10-05T18:50:02Z

@antoinerg it's looking very close! I think the default color logic needs a bit of tweaking though - the inactive color is often too saturated and sometimes too close to the active color, and they're often too prominent in general.

See if you can use Color.contrast without lightAmount or darkAmount, followed by Color.addOpacity (just using different opacity for active and inactive), to make colors that closely match the existing colors when the background is white.

That strategy won't be able to exactly match our current grays (rgba(0,31,95,0.3) and rgba(0,22,72,0.5)), which are tinted a bit blue, unless perhaps you replace Color.defaultLine when returned by Color.contrast with a different color based off those two...

antoinerg · 2018-10-05T19:07:03Z

I followed your suggestion @alexcjohnson in commit db9edd1:

coerce('modebar.bgcolor', Color.addOpacity(layoutOut.paper_bgcolor, 0.5));
var modebarDefaultColor = Color.contrast(Color.rgb(layoutOut.modebar.bgcolor));
coerce('modebar.color', Color.addOpacity(modebarDefaultColor, 0.3));
coerce('modebar.activecolor', Color.addOpacity(modebarDefaultColor, 0.7));

which gives the much improved result seen below:

Do you have any other comments/suggestions?

alexcjohnson · 2018-10-05T20:29:34Z

Indeed, much improved! I'm a little surprised that the red background counts as dark according to Color.contrast, the inactive buttons are a little hard to see there. But... really, who makes a plot with a bright red background? To my eye all the others look good, so I'd say we're ready to go!
💃

jackparmer · 2018-10-05T21:53:53Z

🎉 🥇 🔍

antoinerg added 2 commits October 2, 2018 12:55

add config option modeBarStyle, update logo

1b7c126

fix margins and alignments of modeBar buttons

947eaf2

antoinerg added feature something new status: in progress labels Oct 2, 2018

antoinerg requested a review from alexcjohnson October 2, 2018 18:49

antoinerg commented Oct 2, 2018

View reviewed changes

specify default style in modebar tests

212612b

antoinerg added the status: discussion needed label Oct 2, 2018

increase sizeo of plot.ly logo

3795223

antoinerg added 3 commits October 2, 2018 17:36

fix rendering of logo in IE

e870e34

fix no-multiple-empty-lines

a4d3948

slurp entire SVG, uses DOMParser instead of innerHTML

61d1c7b

etpinard reviewed Oct 2, 2018

View reviewed changes

src/fonts/ploticon/ploticon.svg Show resolved Hide resolved

add build artefact for icons

1d4ac7e

move plot.ly logo on top for vertical modebar

ff9d801

antoinerg added 3 commits October 3, 2018 13:40

move modeBarStyle from config to layout options

397a6de

update modebar tests with modeBarStyle in layout instead of config

baeb1bc

set proper editType on layout attribute modeBar

254f158

etpinard reviewed Oct 3, 2018

View reviewed changes

antoinerg added 2 commits October 3, 2018 16:05

modeBarStyle managed in css, new layout attribute activeIconColor

f9e8777

remove 🐫 in modebar layout attributes

fc47c72

more robust test for modeBar.destroy

0046f12

antoinerg added status: reviewable and removed status: discussion needed labels Oct 4, 2018

place tooltips on the left of vertical modebars

b7c2355

etpinard added this to the v1.42.0 milestone Oct 4, 2018

etpinard reviewed Oct 4, 2018

View reviewed changes

antoinerg added 6 commits October 4, 2018 14:19

in modebar plotly logo has same position in both orientations (v|h)

31d6fb2

fix to change modebar style on relayout

97f25e2

trim whitespace in plotly logo's svg

dc7877b

🔒 down modebar styling option

f5cc0e9

by default modebar colors contrasts with paper_bgcolor

452f9bc

fix default modebar colors

266d63b

antoinerg added status: in progress and removed status: reviewable labels Oct 5, 2018

improve default modebar colors

ff3f9f6

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

improve default modebar color

db9edd1

antoinerg merged commit 88d48c5 into master Oct 5, 2018

antoinerg deleted the 3063-modebar-style branch October 5, 2018 20:32

etpinard mentioned this pull request Oct 30, 2018

Fix modebar <style> update in IE11 #3187

Merged

wbrgss mentioned this pull request Jan 28, 2019

Vertical modebars break plots on IE11 #3489

Closed

etpinard mentioned this pull request Jul 18, 2019

Add showEditInChartStudio config option #4061

Merged

	// These are forbidden in IE only in SVG but since
	// that's 99% of what we do here, we'll forbid them entirely
	// until there's some HTML use case where we need them.
	// (not sure what we'd do then, but we'd think of something!)

	header.on('mouseover', function() {
	header.call(styleOnMouseOver);
	});

	function styleOnMouseOver(item) {
	item.select('rect.' + constants.itemRectClassName)
	.call(Color.fill, constants.hoverColor);
	}

modeBar styling #3068

modeBar styling #3068

Conversation

antoinerg commented Oct 2, 2018 • edited Loading

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

antoinerg commented Oct 2, 2018

antoinerg commented Oct 3, 2018

alexcjohnson commented Oct 3, 2018

antoinerg commented Oct 3, 2018 • edited Loading

alexcjohnson commented Oct 3, 2018

etpinard commented Oct 3, 2018

alexcjohnson commented Oct 3, 2018

etpinard commented Oct 3, 2018

antoinerg commented Oct 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoinerg Oct 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

antoinerg commented Oct 4, 2018

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

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

antoinerg Oct 4, 2018 • edited Loading

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

antoinerg commented Oct 5, 2018

alexcjohnson commented Oct 5, 2018

antoinerg commented Oct 5, 2018 • edited Loading

alexcjohnson commented Oct 5, 2018

jackparmer commented Oct 5, 2018

antoinerg commented Oct 2, 2018 •

edited

Loading

antoinerg commented Oct 3, 2018 •

edited

Loading

antoinerg Oct 3, 2018 •

edited

Loading

antoinerg Oct 4, 2018 •

edited

Loading

antoinerg commented Oct 5, 2018 •

edited

Loading