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

Fix exit animation of notices #18182

Merged
merged 5 commits into from
Mar 24, 2022
Merged

Fix exit animation of notices #18182

merged 5 commits into from
Mar 24, 2022

Conversation

hassaanelgarem
Copy link
Contributor

@hassaanelgarem hassaanelgarem commented Mar 22, 2022

Fixes #18019

Description

  • Fixed issue that caused the change in vertical position of notices to not be animate on dismissal 7604a49
  • Modified the dismissal animation to move the whole notice off-screen instead of moving it to the bottom of the screen.

Normal Animation Speed

Simulator.Screen.Recording.-.iPhone.13.Pro.-.2022-03-22.at.04.40.44.mp4

Slowed down Animation Speed

Simulator.Screen.Recording.-.iPhone.13.Pro.-.2022-03-22.at.04.39.26.mp4

Testing Instructions

  1. Enable quick start for a site
  2. Go back to My Site
  3. Tap Customize Your Site
  4. Tap on a step and follow-through
  5. Observe the animation of replacing a notice with a new notice; it should be smooth.
  6. When done with a step, a notice for the next step will be automatically shown
  7. Press Not now
  8. Observe the notice being dismissed with a smooth animation

Regression Notes

  1. Potential unintended areas of impact
    N/A

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    N/A

  3. What automated tests I added (or what prevented me from doing so)
    N/A

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@hassaanelgarem
Copy link
Contributor Author

@osullivanchris I based the current implementation on the animation you shared here. However, I didn't apply all the exact properties cause I found these were already defined before.

Mainly what's different is:

  • Speed of transition: 1 second instead of 0.3
  • Delay before disappearing: 5 seconds instead of 3
  • Distance moved up and down: This depends on the height of the notice and is not constant. But it moves down till it's no longer visible. And moves up till it's right above the tab bar.

Let me know what you think!

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 22, 2022

You can test the Jetpack changes on this Pull Request by downloading it from AppCenter here with build number: pr18182-d9453f6. IPA is available here. If you need access to this, you can ask a maintainer to add you.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Mar 22, 2022

You can test the WordPress changes on this Pull Request by downloading it from AppCenter here with build number: pr18182-d9453f6. IPA is available here. If you need access to this, you can ask a maintainer to add you.

Copy link
Contributor

@momo-ozawa momo-ozawa left a comment

Choose a reason for hiding this comment

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

Works as described! Looks great ✨

@osullivanchris
Copy link

@hassaanelgarem thanks for all the details! I was just defining in absence of existing definitions. The values you have shared sound fine. The outcome looks really good anyway, much smoother.

I noticed a couple details that felt a bit clunky. They're more about transitions between steps, than the animation, so might be out of scope here.

  • For the homepage task, when I started from the dashboard, the delay after I tap 'Site menu' feels slightly too long. Maybe this applies whenever the user gets 'Tap site menu' first. I haven't checked.
  • On the reader task, on the last step when I tap search, the way the notice disappears behind the keyboard feels a bit off

@hassaanelgarem
Copy link
Contributor Author

For the homepage task, when I started from the dashboard, the delay after I tap 'Site menu' feels slightly too long. Maybe this applies whenever the user gets 'Tap site menu' first.

@osullivanchris This indeed applies whenever "Tap site menu" step is performed. We deliberately add this delay to fix a different issue. Without this delay, or if we even decrease it a bit, the next step behaves weirdly. @momo-ozawa Can speak more about this and how easy it would be to fix it. But I'm guessing it won't be an easy fix.

On the reader task, on the last step when I tap search, the way the notice disappears behind the keyboard feels a bit off.

It does feel off. I will create another issue to track this and will try to address this in a future PR 👍

@momo-ozawa
Copy link
Contributor

We deliberately add this delay to fix a different issue. Without this delay, or if we even decrease it a bit, the next step behaves weirdly. @momo-ozawa Can speak more about this and how easy it would be to fix it. But I'm guessing it won't be an easy fix.

@osullivanchris 👋

Yep, as @hassaanelgarem mentioned this delay is a workaround to prevent the waypoint from flashing several times after landing on the site menu, if the Quick Start tour was triggered from the dashboard. Previously I was using .5 seconds but that wasn't enough of a delay, so I bumped it up to 1 second.

The delay is necessary because we need the site menu to finish reloading before we scroll / display a waypoint on a menu item to prevent the flashing issue. Unfortunately I don't think there's a simple fix for this since it'll require us to re-work the reloading logic for the site menu.

Linking this comment for more context

@osullivanchris
Copy link

@hassaanelgarem @momo-ozawa
thanks for the explanation re; the 1 second delay. Its not a deal breaker for me. Sounds like you had enough trouble to get this working, and its already really good. So we can park that request 😄

Glad if we can do something on reader, search, thanks @hassaanelgarem !

@hassaanelgarem hassaanelgarem merged commit c8a13d7 into trunk Mar 24, 2022
@hassaanelgarem hassaanelgarem deleted the fix/18019-notices-glitch branch March 24, 2022 13:28
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.

Notices are glitching while being dismissed
4 participants