Add scrollToElement command and scrollBeforeClick option #167
Conversation
There was a problem hiding this comment.
Hi @janza thank you for this PR! Looks good other than a small typeo
README.md
Outdated
| @@ -152,6 +152,7 @@ const chromeless = new Chromeless({ | |||
| - [`mousedown(selector: string)`](docs/api.md#api-mousedown) | |||
| - [`mouseup(selector: string)`](docs/api.md#api-mouseup) | |||
| - [`scrollTo(x: number, y: number)`](docs/api.md#api-scrollto) | |||
| - [`scrollToElement(selector: string)`](docs/api.md#api-scrollto) | |||
There was a problem hiding this comment.
Looks like this is linking to the wrong place. Should be docs/api.md#api-scrolltoelement.
There was a problem hiding this comment.
@janza Hm.. I noticed another thing. In #15 it's mentioned in #15 (comment) that we should scroll past the element until it's completely within the viewport, however I'm not sure if your PR incorporates that since you're scrolling to the top-left of the element.
To address this, it might be as simple as, instead of scrolling to the top-left of the element rect, scroll to the bottom-right? Although this might not cover the case where you are scrolling horizontally. @joelgriffith @timsuchanek?
|
What it does is it aligns the top left point of the viewport to the top left point of the element to scroll to, so the whole element will be in the viewport (unless it's bigger than viewport). |
|
I'm digging deep back into the jQuery days (and anchor hrefs), which do scroll that element to the top of the viewport panel. I feel that most folks will be somewhat expecting this just due to the nature of scroll-to's elsewhere. |
docs/api.md
Outdated
|
|
||
| ### scrollToElement(selector: string): Chromeless<T> | ||
|
|
||
| Scroll to location of element. |
There was a problem hiding this comment.
Might add a note that the expected behavior is the element will be at the top of viewport just to remove that uncertainty
closes #15