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.

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

Add scrollTo selector #774

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Add scrollTo selector #774

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
b9f926f
Update clickAndHoverHelper.js
aliu145 May 15, 2018
b9fdcd3
Update clickAndHoverHelper.js
aliu145 May 15, 2018
0712adf
Merge pull request #2 from aliu145/anna.liu/add-scrollTo-selector-1
aliu145 May 15, 2018
7d0b902
Update README.md
aliu145 May 15, 2018
a394746
Merge pull request #3 from aliu145/anna.liu/add-scrollTo-selector-2
aliu145 May 15, 2018
bd77957
Adding test for scrollToSelector
May 17, 2018
019e1d9
Update index.html
aliu145 May 17, 2018
2a019aa
Update backstop_features.js
aliu145 May 17, 2018
24a52dd
Merge pull request #4 from aliu145/master
aliu145 May 17, 2018
7d9c350
Merge branch 'anna.liu/add-scrollTo-selector' of https://github.com/a…
Jun 25, 2018
e4c2770
Adding tests for scrollToSelector
Jun 26, 2018
c0640ee
update test label
Jul 12, 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
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ removeSelectors // Array of selectors set to display: none
onReadyScript // After the above conditions are met -- use this script to modify UI state prior to screen shots e.g. hovers, clicks etc.
hoverSelector // Move the pointer over the specified DOM element prior to screen shot (available with default onReadyScript)
clickSelector // Click the specified DOM element prior to screen shot (available with default onReadyScript)
scrollToSelector // Scrolls the specified DOM element into view prior to screen shot (available with default onReadyScript)
postInteractionWait // Wait for a selector after interacting with hoverSelector or clickSelector (optionally accepts wait time in ms. Idea for use with a click or hover element transition. available with default onReadyScript)
selectors // Array of selectors to capture. Defaults to document if omitted. Use "viewport" to capture the viewport size. See Targeting elements in the next section for more info...
selectorExpansion // See Targeting elements in the next section for more info...
Expand Down
10 changes: 10 additions & 0 deletions capture/engine_scripts/chromy/clickAndHoverHelper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
module.exports = function (chromy, scenario) {
var hoverSelector = scenario.hoverSelector;
var clickSelector = scenario.clickSelector;
var scrollToSelector = scenario.scrollToSelector;
var postInteractionWait = scenario.postInteractionWait; // selector [str] | ms [int]

if (hoverSelector) {
Expand All @@ -17,6 +18,15 @@ module.exports = function (chromy, scenario) {
.wait(clickSelector)
.click(clickSelector);
}

if (scrollToSelector) {
chromy
.wait()
Copy link
Owner

Choose a reason for hiding this comment

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

what does this line do?

.evaluate(`_scrollToSelector = '${scrollToSelector}'`)
Copy link
Owner

Choose a reason for hiding this comment

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

I dont think you need this line.

.evaluate(function() {
document.querySelector(_scrollToSelector).scrollIntoView();
}, scrollToSelector);
Copy link
Owner

Choose a reason for hiding this comment

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

I think you want this...

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

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

}

if (postInteractionWait) {
chromy.wait(postInteractionWait);
Expand Down
10 changes: 10 additions & 0 deletions capture/engine_scripts/puppet/clickAndHoverHelper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
module.exports = async (page, scenario) => {
var hoverSelector = scenario.hoverSelectors || scenario.hoverSelector;
var clickSelector = scenario.clickSelectors || scenario.clickSelector;
var scrollToSelector = scenario.scrollToSelectors || scenario.scrollToSelector;
var postInteractionWait = scenario.postInteractionWait; // selector [str] | ms [int]

if (hoverSelector) {
Expand All @@ -16,6 +17,15 @@ module.exports = async (page, scenario) => {
await page.click(clickSelectorIndex);
}
}

if (scrollToSelector) {
for (const scrollToSelectorIndex of [].concat(scrollToSelector)) {
Copy link
Owner

Choose a reason for hiding this comment

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

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

await page.waitFor(scrollToSelectorIndex);
await page.evaluate(scrollToSelectorIndex => {
document.querySelector(scrollToSelectorIndex).scrollIntoView();
}, scrollToSelectorIndex);
}
}

if (postInteractionWait) {
await page.waitFor(postInteractionWait);
Expand Down
3 changes: 3 additions & 0 deletions index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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">
Copy link
Owner

Choose a reason for hiding this comment

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

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

Puppies!
</a>
</p>
</div>
</div>
Expand Down
6 changes: 6 additions & 0 deletions test/configs/backstop_features.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,12 @@ module.exports = {
selectors: ['.moneyshot'],
misMatchThreshold: 5.0,
requireSameDimensions: false
},
{
label: 'scrollTo',
url: '../../index.html',
scrollToSelector: '.puppy-link',
selectors: ['puppy-link']
Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

ohhhh gotcha! OK I will work on adding that

}
],
paths: {
Expand Down