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.

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

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

Conversation

sammuell
Copy link
Contributor

This PR adds an option to disable any transitions of the tour (on by default). It also implements smooth page scrolling (issue #1719)

Copy link
Contributor

@afshinm afshinm left a 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({
Copy link
Contributor

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

Copy link
Contributor Author

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()

Copy link
Contributor

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?

Copy link

@XaviTorello XaviTorello left a 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;

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants