Skip to content

responsive tests: check size of DOM elements with getBoundingClientRect() #3484

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

Merged
merged 1 commit into from
Jan 25, 2019

Conversation

antoinerg
Copy link
Contributor

This makes for more robust tests since a style attribute can override the width and height attribute of a DOM element. We now check for both to be safe.

@plotly/core-contributors

@@ -553,7 +553,7 @@ describe('config argument', function() {

describe('responsive figure', function() {
var gd;
var data = [{x: [1, 2, 3, 4], y: [5, 10, 2, 8]}];
var data = [{type: 'scatter', x: [1, 2, 3, 4], y: [5, 10, 2, 8]}];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is scatter by default, but it is clearer if we make it explicit.

var mainSvgs = document.getElementsByClassName('main-svg');
checkElementsSize(mainSvgs, elWidth / 2, elHeight / 2);

var canvases = document.getElementsByTagName('canvas');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checking the size of the canvas elements sandwiched between the SVG elements is also very important.

@VeraZab
Copy link

VeraZab commented Jan 25, 2019

@antoinerg , maybe use @plotly/plotly_js ? @plotly/core-contributors notifies a lot more people, who don't necessarily work with plotly.js :)

@etpinard
Copy link
Contributor

💃 thanks!

Next time though:

@antoinerg antoinerg merged commit e94ee3d into master Jan 25, 2019
@antoinerg antoinerg deleted the improve-responsive-tests branch January 30, 2019 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants