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

Add step transition animation in ABR setup #7292

Closed
mxbclang opened this issue Jul 12, 2023 · 4 comments
Closed

Add step transition animation in ABR setup #7292

mxbclang opened this issue Jul 12, 2023 · 4 comments
Labels
Module: AdSense Google AdSense module related issues P2 Low priority Type: Enhancement Improvement of an existing feature

Comments

@mxbclang
Copy link

mxbclang commented Jul 12, 2023

Feature Description

As reported by @kuasha420 in Bug Bash:

There should be a "subtle" transition animation when the step is changed from 1 to 2, currently it feels abrupt.

See https://mui.com/material-ui/react-stepper/#vertical-stepper for an example of it.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The Stepper component should have a transitional animation when changing the current step. This should be modelled on the MUI vertical stepper linked in the description - the new step should slide up into view, replacing the previous step.
  • This animation should be evident on the Ad Blocking Recovery setup page, which is currently the only usage of the Stepper component in the plugin.

Implementation Brief

As discussed here it doesn't seem to be possible to transition an HTML element from zero height to the height of its content using CSS only. Therefore we take an approach using JS to set the height to transition to when setting a step to be active. Furthermore as discussed here, we take the approach of setting the height to 0 for a non-active step, which means we can determine its content height by using its scrollHeight property.

  • Update the Step component so it unconditionally renders its content, adding a CSS class to the content div for the active status.
  • Use the principles outlined above to implement the transition.
  • Additionally, apply visibility: hidden to the non-active steps' content in order to hide their hidden content from screen readers and prevent it from being tabbed into.
  • Refer to/finish off this draft PR.

Test Coverage

  • Update the Stepper tests to accommodate these changes.

QA Brief

  • Set up Site Kit with the AdSense module.
  • Go to the Ad Blocking Recovery setup.
  • When going through the steps, verify that there is an animation as described in the ACs in between each step.
  • Verify the Storybook story for Stepper and ensure that has the animation as well.

Changelog entry

  • Add a step transition animation to the Ad Blocking Recovery setup widget.
@mxbclang mxbclang added P2 Low priority Type: Enhancement Improvement of an existing feature labels Jul 12, 2023
@mxbclang mxbclang added the Module: AdSense Google AdSense module related issues label Jul 20, 2023
@techanvil techanvil self-assigned this Aug 1, 2023
@techanvil techanvil removed their assignment Sep 4, 2023
@tofumatt tofumatt assigned tofumatt and unassigned tofumatt Sep 7, 2023
@tofumatt
Copy link
Collaborator

tofumatt commented Sep 7, 2023

IB ✅

@techanvil techanvil added the Next Up Issues to prioritize for definition label Nov 15, 2023
@nfmohit nfmohit self-assigned this Nov 21, 2023
@nfmohit nfmohit removed their assignment Nov 22, 2023
@techanvil techanvil self-assigned this Nov 22, 2023
techanvil added a commit that referenced this issue Nov 22, 2023
@techanvil techanvil removed their assignment Nov 22, 2023
@wpdarren wpdarren self-assigned this Nov 22, 2023
@wpdarren
Copy link
Collaborator

wpdarren commented Nov 23, 2023

QA Update: ❌

@nfmohit I have two observations to highlight.

1. When I click on the Set up now button, there's no animation on the button, the AdBlocking Recovery CTA disappears, and the dashboard loads. Then a few seconds later I am taken to the first step of the setup.

The issue reported above is not a regression, so I will create a new ticket for this.

  1. On the first step of the set up there are no CTAs or content in between the two sections. When I click on the cancel link though they flash for a few seconds and then I am taken back to the dashboard as expected.

The screencast below showcases both issues.

abr.mp4

@nfmohit
Copy link
Collaborator

nfmohit commented Nov 23, 2023

Thank you @wpdarren. I've added a follow-up PR #7910 that aims to address the second issue.

@tofumatt tofumatt assigned tofumatt and nfmohit and unassigned tofumatt Nov 23, 2023
tofumatt added a commit that referenced this issue Nov 24, 2023
Fix `AdBlockingRecoveryApp` setup status unresolved issue
@tofumatt tofumatt assigned wpdarren and unassigned nfmohit Nov 24, 2023
@mohitwp mohitwp assigned mohitwp and unassigned wpdarren Nov 27, 2023
@mxbclang mxbclang removed the Next Up Issues to prioritize for definition label Nov 28, 2023
@mohitwp
Copy link
Collaborator

mohitwp commented Nov 28, 2023

QA Update ✅

  • Tested on dev environment.
  • Verified issue reported above is now resolved.
  • Verified transition animation is implemented successfully ABR set up steps.
Recording.670.mp4
Recording.672.mp4

@mohitwp mohitwp removed their assignment Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: AdSense Google AdSense module related issues P2 Low priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

7 participants