-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Disable tour animation #1728
base: master
Are you sure you want to change the base?
Disable tour animation #1728
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! The only missing piece seems to be the tests which should be fairly straightforward to add. Happy to help
window.scrollBy( | ||
0, | ||
rect.top - | ||
window.scrollBy({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great if we add some Cypress tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. What would you test in this case? It still uses the same function window.scrollBy()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay @sammuell. We usually have a delay() between each step in the Cypress tests to make sure the transition is complete. Now that the transition can be enabled/disabled, we can probably have a test without that delay(), take a screenshot and that should capture the tooltip in its correct position. Would that be a useful test case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ty dudes! In which state is this PR? I think that can be quite useful to integrate the possibility to disable tour animations ASAP.
I made a proposal to extend customization capabilities via new Tour setting tooltipTimeoutMs
@@ -246,6 +246,7 @@ export default async function _showElement(targetElement) { | |||
removeShowElement(); | |||
|
|||
//we should wait until the CSS3 transition is competed (it's 0.3 sec) to prevent incorrect `height` and `width` calculation | |||
let timeout = (self._options.tourAnimation) ? 350 : 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be amazing if the timeout can also be configurable, maybe a new prop tooltipTimeoutMs
to customize the timemout ms can be defined
This PR adds an option to disable any transitions of the tour (on by default). It also implements smooth page scrolling (issue #1719)