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

Fixes #3430 - Improve sliding animations on slow mobile phones #3484

Merged
merged 1 commit into from
Sep 21, 2020

Conversation

ksy36
Copy link
Contributor

@ksy36 ksy36 commented Sep 21, 2020

In this PR I've changed the way steps are animated to make it perform faster on slow mobile phones

@@ -10,380 +10,18 @@ <h1 class="headline-1">Report Site Issue</h1>
novalidate
>
<h3 class="page-heading">Filing a web compatibility issue</h3>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

pretty much no changes here, just split this file into smaller html files

Copy link
Member

Choose a reason for hiding this comment

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

yeah, makes it easier to digest.

}
100% {
margin-top: var(--presumed-high-step-height);
opacity: 0;
Copy link
Contributor Author

@ksy36 ksy36 Sep 21, 2020

Choose a reason for hiding this comment

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

slidedown and slideup keyframes are replaced with js animations and instead of margin-top, translateY is being modified (that helped to speed things up)

@ksy36
Copy link
Contributor Author

ksy36 commented Sep 21, 2020

r? @miketaylr

@ksy36 ksy36 requested a review from miketaylr September 21, 2020 02:17
Copy link
Member

@miketaylr miketaylr left a comment

Choose a reason for hiding this comment

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

@ksy36 should we deploy this to staging to see if it makes a difference on your phone?

@@ -110,7 +110,7 @@ registerSuite("Reporting with wizard", {
// Click on "Desktop site instead of mobile site"
document.querySelector("[for=problem_category-0]").click();
})
.sleep(1000)
.sleep(1500)
Copy link
Member

Choose a reason for hiding this comment

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

increasing timeouts is always a code smell that a test should probably be re-written. however, i'm a fan of getting things to pass and kicking the timeout can down the road a bit. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, we could probably add a function to check whether animation and scrolling to a step is finished instead of using timeouts.. I'll file a bug for that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

complete: () => {
container.addClass("open");
scrollToElement(domElement);
if (cb) cb();
Copy link
Member

@miketaylr miketaylr Sep 21, 2020

Choose a reason for hiding this comment

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

I am so disappointed that prettier allows this style. 😞

(lol, just a personal thing, no need for a change)

@@ -10,380 +10,18 @@ <h1 class="headline-1">Report Site Issue</h1>
novalidate
>
<h3 class="page-heading">Filing a web compatibility issue</h3>
Copy link
Member

Choose a reason for hiding this comment

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

yeah, makes it easier to digest.

@ksy36
Copy link
Contributor Author

ksy36 commented Sep 21, 2020

I've made the changes while testing on my phone on a local ip, so it should be good :)

@miketaylr miketaylr merged commit d02b58b into master Sep 21, 2020
@miketaylr miketaylr deleted the issue/3430/1 branch September 21, 2020 16:09
@ksy36
Copy link
Contributor Author

ksy36 commented Sep 21, 2020

Thanks!

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

Successfully merging this pull request may close these issues.

2 participants