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

Hiding dashboard cards: fix scroll position #20454

Merged
merged 1 commit into from
May 31, 2023
Merged

Hiding dashboard cards: fix scroll position #20454

merged 1 commit into from
May 31, 2023

Conversation

kean
Copy link
Contributor

@kean kean commented Mar 30, 2023

Related issue: #20296

Fixes an issue with an incorrect scroll position after hiding multiple cards in the Personalize Home Tab screen and then turning them back on. The issue was reported by @guarani here (see the linked comment for the video).

To test:

  • Open Jetpack and enable the "Personalize Home Tab" flag
  • Tap "Personalize Home Tab" on the bottom of the site's Dashboard
  • Turn all cards off, then turn some of them back off
  • Go back the Dashboard
  • Verify that the scroll view has normal scroll position: cards are visible

Regression Notes

  1. Potential unintended areas of impact.
    Site Dashboard.
  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Manual testing
  3. What automated tests I added (or what prevented me from doing so)
    No

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.

@kean kean marked this pull request as draft March 31, 2023 13:47
return
}

self?.scroll(scrollView, to: position)
Copy link
Contributor Author

@kean kean Mar 31, 2023

Choose a reason for hiding this comment

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

The scroll(_:to:) code was added in the following PR as a workaround. Unfortunately, it breaks the new personalization feature (see the comment).

I tried removing the workaround to see if it's still needed and was not able to reproduce the issue it's supposed to be fixing, so it should be good to merge.

@kean kean marked this pull request as ready for review March 31, 2023 18:14
@momo-ozawa momo-ozawa self-requested a review May 31, 2023 15:31
@momo-ozawa momo-ozawa added this to the 22.6 milestone May 31, 2023
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! I kicked off a CI build - if it's green all good to merge 👍

@kean kean merged commit 9c58ff9 into wordpress-mobile:trunk May 31, 2023
@kean kean deleted the feature/personalize-home-tab-fix-scroll-position branch May 31, 2023 18:43
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.

2 participants