Add scrollTo selector #774

aliu145 · 2018-05-15T16:48:26Z

No description provided.

Update clickAndHoverHelper.js

Update README.md

aliu145 · 2018-05-15T17:23:37Z

@garris I'm not sure why the update to README.md failed Docker Cloud

garris

This looks great. Would you please add a test for this in https://github.com/garris/BackstopJS/blob/master/test/configs/backstop_features.js

Maybe you can sneak an non-obvious nested element inside the index.html that we use for testing. Then please run the smoke test to insure it’s working as expected.

Just let me know if you have any questions.

Adding test

aliu145 · 2018-05-17T23:04:35Z

I added a test, but wasn't able to run it since I'm running into issues installing BackstopJS dependencies (puppeteer)... would you be able to help pull my branch and run it?

garris · 2018-05-18T04:08:34Z

No problem — will do this tomorrow.

Question— does puppeteer install on windows correctly if you just install with npm install puppeteer into some directory? I am not clear if there is a problem with puppeteer or the backstop installation.

aliu145 · 2018-05-24T16:16:42Z

@garris - Hello! any update on this? When will it be able to be merged in? Thanks.

garris

Hi Anna, Thank you very much for the PR. I added a few comments. It might take a bit more effort in some areas but I hope you stick with it. Please reach out if you have questions. Best, Garris

garris · 2018-05-24T23:35:15Z

capture/engine_scripts/chromy/clickAndHoverHelper.js

+
+  if (scrollToSelector) {
+    chromy
+      .wait()


what does this line do?

garris · 2018-05-24T23:37:10Z

capture/engine_scripts/chromy/clickAndHoverHelper.js

+  if (scrollToSelector) {
+    chromy
+      .wait()
+      .evaluate(`_scrollToSelector = '${scrollToSelector}'`)


I dont think you need this line.

garris · 2018-05-24T23:41:11Z

capture/engine_scripts/chromy/clickAndHoverHelper.js

+      .evaluate(`_scrollToSelector = '${scrollToSelector}'`)
+      .evaluate(function() {
+        document.querySelector(_scrollToSelector).scrollIntoView();
+      }, scrollToSelector);


I think you want this...

.evaluate(function(scrollToSelector){ document.querySelector(scrollToSelector).scrollIntoView(); }, scrollToSelector);

You probably also need to guard against scrollToSelector not being found.

garris · 2018-05-24T23:43:42Z

capture/engine_scripts/puppet/clickAndHoverHelper.js

@@ -16,6 +17,15 @@ module.exports = async (page, scenario) => {
      await page.click(clickSelectorIndex);
    }
  }
+
+  if (scrollToSelector) {
+    for (const scrollToSelectorIndex of [].concat(scrollToSelector)) {


Do you intend scrollToSelector to take an array value? or just a string? Array feels like it would produce some noisy results.

garris · 2018-05-24T23:47:53Z

index.html

@@ -46,6 +46,9 @@ <h1>Visual regression testing for web apps.</h1>
        <p class="getItBlock col-10">
          <span class="cta"><a href="https://github.com/garris/BackstopJS" class="github-link"><img src="assets/github-icon.png" class="github-icon" alt="Get it on GitHub"></a></span>
          <span class="cta"><a href="https://github.com/garris/BackstopJS">Get it on github!</a></span>
+          <a href="https://github.com/garris/BackstopJS" class="puppy-link">


Is this going to show up on the backstop brochure page? It's a bit of a sequitur 🤔

garris · 2018-05-24T23:51:01Z

test/configs/backstop_features.js

+      label: 'scrollTo',
+      url: '../../index.html',
+      scrollToSelector: '.puppy-link',
+      selectors: ['puppy-link']


I thought this feature was intended to test a child-element inside a scrollable div? I thought the below-the-fold elements that were direct children of the body tag already worked correctly? Am I missing something here?

It is... how can I write a backstop_features test that can imitate this? Do I need to add a scrollable div to index.html?

In this case — I think it’s best that you create a folder in the test directory, maybe call it “special_cases” or something like that. And in there you can add an html file which easily simulates the issue. For that scenario you can point to that file. Does that make sense?

I just thought — what might be better instead is to add a folder to the examples directory and call it “scrolling_cases”. In there you can add an html file which simulated the issue and a backstop config which demonstrates how to test the case. You can add a short readme with a sentence which explains the issue and how to resolve it so other users can learn from your example.

ohhhh gotcha! OK I will work on adding that

garris · 2018-05-25T00:01:10Z

Hi @aliu145 -- very sorry -- I had actually started a review but ran out of time and never submitted the comments. The review is up. Please take a look and feel free to reach out if you have questions.

Also -- did you finally resolve your installation issue? Generally we like to test our work before posting a PR.

Thanks!

…liu145/BackstopJS into anna.liu/add-scrollTo-selector

aliu145 · 2018-06-26T17:30:41Z

Hmm, as I'm testing this, it's working for puppeteer:

But not for chromy:

Do you have any idea why that might be?

garris

Thanks for this — I’ll be in Japan in holiday for two weeks. Will review as soon as I can. Cheers, Garris

garris · 2018-06-26T19:47:20Z

See my comment for the Chromy script issue.

aliu145 · 2018-06-26T22:27:40Z

capture/engine_scripts/chromy/clickAndHoverHelper.js

+      .evaluate(`_scrollToSelector = '${scrollToSelector}'`)
+      .evaluate(function() {
+        document.querySelector(_scrollToSelector).scrollIntoView();
+      }, scrollToSelector);


Hi @garris, I made these updates but it's still not working (ie the screenshot shows that it did not properly scroll the container)

aliu145 · 2018-07-12T18:55:50Z

@garris , I am still seeing issue where the same code is able to scroll container with puppeteer, but doesn't work with chromy :/

Here is the chromy script:
` if (scrollToSelector) {

chromy

  .wait(scrollToSelector)

  .evaluate(function () {

    document.querySelector(scrollToSelector).scrollIntoView();

  }, scrollToSelector);

}`

Do you have any suggestions?

garris · 2018-07-13T08:41:58Z

Ok Ms Anna, Fixed and Merged!

Final changes here 👉 128c647

Please test & validate.

aliu145 added 5 commits May 15, 2018 09:34

Update clickAndHoverHelper.js

b9f926f

Update clickAndHoverHelper.js

b9fdcd3

Merge pull request #2 from aliu145/anna.liu/add-scrollTo-selector-1

0712adf

Update clickAndHoverHelper.js

Update README.md

7d0b902

Merge pull request #3 from aliu145/anna.liu/add-scrollTo-selector-2

a394746

Update README.md

aliu145 mentioned this pull request May 15, 2018

Need way to scroll container #765

Closed

garris requested changes May 15, 2018

View reviewed changes

Anna Liu and others added 4 commits May 17, 2018 15:57

Adding test for scrollToSelector

bd77957

Update index.html

019e1d9

Update backstop_features.js

2a019aa

Merge pull request #4 from aliu145/master

24a52dd

Adding test

garris requested changes May 24, 2018

View reviewed changes

Anna Liu added 2 commits June 25, 2018 16:40

Merge branch 'anna.liu/add-scrollTo-selector' of https://github.com/a…

7d9c350

…liu145/BackstopJS into anna.liu/add-scrollTo-selector

Adding tests for scrollToSelector

e4c2770

garris reviewed Jun 26, 2018

View reviewed changes

aliu145 commented Jun 26, 2018

View reviewed changes

update test label

c0640ee

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add scrollTo selector #774

Add scrollTo selector #774

aliu145 commented May 15, 2018

aliu145 commented May 15, 2018 •

edited

Loading

garris left a comment

aliu145 commented May 17, 2018

garris commented May 18, 2018

aliu145 commented May 24, 2018

garris left a comment

garris May 24, 2018

garris May 24, 2018

garris May 24, 2018

garris May 24, 2018

garris May 24, 2018

garris May 24, 2018

garris May 24, 2018

aliu145 Jun 25, 2018

garris Jun 26, 2018

garris Jun 26, 2018

aliu145 Jun 26, 2018

garris commented May 25, 2018

aliu145 commented Jun 26, 2018

garris left a comment

garris commented Jun 26, 2018

aliu145 Jun 26, 2018

aliu145 commented Jul 12, 2018 •

edited

Loading

garris commented Jul 13, 2018 •

edited

Loading

Add scrollTo selector #774

Are you sure you want to change the base?

Add scrollTo selector #774

Conversation

aliu145 commented May 15, 2018

aliu145 commented May 15, 2018 • edited Loading

garris left a comment

Choose a reason for hiding this comment

aliu145 commented May 17, 2018

garris commented May 18, 2018

aliu145 commented May 24, 2018

garris left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garris commented May 25, 2018

aliu145 commented Jun 26, 2018

garris left a comment

Choose a reason for hiding this comment

garris commented Jun 26, 2018

Choose a reason for hiding this comment

aliu145 commented Jul 12, 2018 • edited Loading

garris commented Jul 13, 2018 • edited Loading

aliu145 commented May 15, 2018 •

edited

Loading

aliu145 commented Jul 12, 2018 •

edited

Loading

garris commented Jul 13, 2018 •

edited

Loading