Skip to content
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.

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

implement watermark config option #3280

Merged
merged 2 commits into from
Nov 23, 2018
Merged

implement watermark config option #3280

merged 2 commits into from
Nov 23, 2018

Conversation

antoinerg
Copy link
Contributor

Closes #3268 exactly as described/requested by @nicolaskruchten's in his first comment #3268 (comment)

@@ -5,12 +5,12 @@
z-index: 1001;
}

.modebar--hover {
.modebar--hover > :not(.watermark) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently :not is supported IE9+: https://caniuse.com/#search=%3Anot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CSS child selector should also work in IE9+. The reason we use it is because we cannot override the opacity of a child if the parent has a CSS opacity.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice find. But we should test this out in IE9 and IE11 (from browserstack) before merging this thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works in IE11:
ie11_watermark

IE9/10 doesn't work on master 😐
ie9_fail_master2

I will open an issue for the latter.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh wait, right the main plotly.js bundle doesn't work in ie9.

You'll need to use plotly-basic.js:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works in IE9 🎉

ie9_watermark

About using plotly-basic.js in IE9 versus plotly.js: is this documented somewhere? If not I should open an issue over at the documentation project.

Copy link
Contributor

Choose a reason for hiding this comment

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

More precisely, all SVG-based traces should work in IE9.

We could maybe add some info here ->

https://github.com/plotly/plotly.js/tree/master/dist#to-support-ie9

@@ -133,6 +133,9 @@ module.exports = {
// add the plotly logo on the end of the mode bar
displaylogo: true,

// watermark the images with the company's logo
watermark: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Yesss! false by default 👌

@etpinard
Copy link
Contributor

etpinard commented Nov 22, 2018

Looking good!

It should be possible to add the a few test cases in

https://github.com/plotly/plotly.js/blob/master/test/jasmine/tests/modebar_test.js

to 🔒 down the new config.watermark logic.

@antoinerg
Copy link
Contributor Author

@etpinard I think df10f70 may be sufficient but let me know if it's not 🤔

@etpinard
Copy link
Contributor

💃 nicely done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants