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

[Home Page Picker] Bug fixes #15360

Merged
merged 9 commits into from
Nov 24, 2020

Conversation

chipsnyder
Copy link
Contributor

@chipsnyder chipsnyder commented Nov 20, 2020

Fixes #15303 (comment)

This takes care of a few bug fixes I found while doing a comprehensive test. I'll spell out each one here.

Swipe to dismiss should be disabled

On a device running iOS 13 or newer:

  1. Navigate to the Create WordPress.com site
    • My Sites > ➕ > Create WordPress.com site
  2. Try to swipe away the screen on Choose a Design, Choose a Domain, and Site Confirmation
    • Expect to not be able to swipe away the screens
  3. Try to swipe away the screen on Choose a Design > Preview
    • Expect to be able to swipe away the screens

Header jumping on Choose a Domain

Original issues

Noticed on:

  • iPad Air 11.4

Steps

  1. Navigate to Choose a Domain
  2. Search for results

Notice the header will sometimes expand

Steps

  1. Navigate to Choose a Domain
  2. Scroll to collapse the header
  3. Search for results
  4. Select clear
  5. Search for results
    • Expect the header to maintain state

PR submission checklist:

  • I have considered adding unit tests where possible.
  • 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.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Nov 20, 2020

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

@@ -44,13 +44,15 @@ final class SiteCreationWizardLauncher {
if FeatureFlag.siteCreationHomePagePicker.enabled {
if #available(iOS 13.0, *) {
wizardContent.modalPresentationStyle = .pageSheet
wizardContent.isModalInPresentation = true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This prevents the ability to swipe the modal away.

@@ -32,7 +32,6 @@
<viewLayoutGuide key="safeArea" id="njF-e1-oar"/>
<constraints>
<constraint firstItem="hQm-TV-IDW" firstAttribute="leading" secondItem="njF-e1-oar" secondAttribute="leading" constant="16" id="QB9-fd-Zsn"/>
<constraint firstItem="hQm-TV-IDW" firstAttribute="centerY" secondItem="njF-e1-oar" secondAttribute="centerY" id="eud-Yr-hp5"/>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Saw some conflicting constraint errors in the console

guard stashedOffset == nil else {
restoreContentOffsetIfNeeded(scrollView)
return
}
Copy link
Contributor Author

@chipsnyder chipsnyder Nov 20, 2020

Choose a reason for hiding this comment

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

These changes fix the jumping header. stashedOffset will be reset to nil as soon as a user starts to scroll. (This is in an earlier PR in scrollViewWillBeginDragging)

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Nov 20, 2020

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

Copy link
Contributor

@mkevins mkevins left a comment

Choose a reason for hiding this comment

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

Hi Chip, I tested the PR by first reproducing the issues, and then testing the steps with the changes here on both iPhone 11 (physical device) and iPhone Air (simulator). Everything seems to be working correctly now, but I observed a different issue on the domains screen. It seems that sometimes the results can be partially hidden:

I also noticed that sometimes there are no results for a query that other times has results.. not sure if there is a race condition there 🤔 ?

And one minor nit-pick: the modal seems to move a bit (vertically) from one step to the next. Is this expected?

@chipsnyder
Copy link
Contributor Author

chipsnyder commented Nov 23, 2020

but I observed a different issue on the domains screen. It seems that sometimes the results can be partially hidden:

Good testing! The only way I was able to recreate this is by:

  1. Start with blank results
  2. Enter a search that will return results
  3. Swipe up so that the results populate while the view is scrolling
  4. Notice error

Let me know if you're seeing it with a different set of steps. I have an idea on how to fix this though so I'll investigate that today.

I also noticed that sometimes there are no results for a query that other times has results.. not sure if there is a race condition there 🤔 ?

I noticed some inconsistent behavior here as well but I also started seeing that before we made changes to this screen. It's also different between Android and iOS on the number of results and that Android includes a notice if there is an exact match. Outside of the UI that surrounds the search results, I left the actual search behavior unchanged. In my testing, the inconsistencies were matching what's currently live so I didn't want to hold up the feature while investigating that behavior. I'll create a GH issue today so that we can track that work and mark out the differences. I agree though there is probably a race condition going on with the throttle.

And one minor nit-pick: the modal seems to move a bit (vertically) from one step to the next. Is this expected?

Yeah, I'm not a big fan of this either but this is the system adjusting the modal to make room for the keyboard. Since the Search box is auto-selected it shifts up while animating in. In your video, it looks like the simulator is using the hardware keyboard setting so the soft keyboard doesn't pop, just the toolbar, so the shift is smaller. But it's noticeable about the cause when using a soft keyboard:

@chipsnyder
Copy link
Contributor Author

chipsnyder commented Nov 23, 2020

@mkevins I believe I solved the issue RE: the partially hidden results now: 566ce5c

I also included the Gutenberg version bump for @geriux's Group block background color support

Copy link

@antonis antonis left a comment

Choose a reason for hiding this comment

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

Tested on iPad Air and iPhone 11 (simulators) and everything seems to work as expected 🎉
Nice work Chip 👍

@chipsnyder
Copy link
Contributor Author

Just FYI to cut back on the number of builds to the enable PR I included the wording changes mentioned in #15303 (comment) as part of this PR 86bf7cf

@chipsnyder
Copy link
Contributor Author

Updated this branch with the changes from #15365 and #15376

@chipsnyder
Copy link
Contributor Author

I also included a fix here (3d2a785) where the results could update after you made a selection but update and not include that selection.

Steps

  1. Type a bit of text slowly to start getting some results
  2. add more text
  3. Make a selection before the results update.
    Expect for your selection to be cleared if the new results don't contain your original selection

@chipsnyder
Copy link
Contributor Author

chipsnyder commented Nov 23, 2020

I also noticed that sometimes there are no results for a query that other times has results.. not sure if there is a race condition there 🤔 ?

Investigating this a bit more, I think there might be some inconsistencies on the backend as well. Possibly related to the size of the requested results?

If I make the request: https://public-api.wordpress.com/rest/v1.1/domains/suggestions?locale=en&query=test&segment_id=1
I get:

{
    "error": "empty_results",
    "message": "No available domains for that search."
}

However, if I add quantity like: https://public-api.wordpress.com/rest/v1.1/domains/suggestions?locale=en&query=test&segment_id=1&quantity=20

I'll get about 19 results. I'm interested in the results you're seeing @mkevins. if you hook up a proxy does the app reflect the results from the last search that was performed?

Here's the issue to track this #15378

@mkevins
Copy link
Contributor

mkevins commented Nov 24, 2020

Good testing! The only way I was able to recreate this is by:

  1. Start with blank results
  2. Enter a search that will return results
  3. Swipe up so that the results populate while the view is scrolling
  4. Notice error

Let me know if you're seeing it with a different set of steps. I have an idea on how to fix this though so I'll investigate that today.

These are the same steps I used too, and I didn't notice it otherwise.

I noticed some inconsistent behavior here as well but I also started seeing that before we made changes to this screen. It's also different between Android and iOS on the number of results and that Android includes a notice if there is an exact match. Outside of the UI that surrounds the search results, I left the actual search behavior unchanged. In my testing, the inconsistencies were matching what's currently live so I didn't want to hold up the feature while investigating that behavior. I'll create a GH issue today so that we can track that work and mark out the differences. I agree though there is probably a race condition going on with the throttle.

That makes sense, and good catch on the differences between Android and iOS. I think it makes sense to move forward, and resolve the query issues separately.

Yeah, I'm not a big fan of this either but this is the system adjusting the modal to make room for the keyboard. Since the Search box is auto-selected it shifts up while animating in. In your video, it looks like the simulator is using the hardware keyboard setting so the soft keyboard doesn't pop, just the toolbar, so the shift is smaller. But it's noticeable about the cause when using a soft keyboard:

Ah, thanks for the explanation! In that case, I don't think there is anything to change there 😄 .

@mkevins
Copy link
Contributor

mkevins commented Nov 24, 2020

I'll get about 19 results. I'm interested in the results you're seeing

I tried a few times, and I get no results without the quantity parameter, same as you. With quantity=20, I get mixed results.. sometimes ~15, sometimes ~18, so I guess it changes each time.

if you hook up a proxy does the app reflect the results from the last search that was performed?

Can you clarify this part? Do you mean, to running the app via a sandboxed API (or any other steps)? To be clear, the above results were on the production API, and I observed that even by merely clicking the links in your comment above.

@chipsnyder
Copy link
Contributor Author

if you hook up a proxy does the app reflect the results from the last search that was performed?
Can you clarify this part? Do you mean, to running the app via a sandboxed API (or any other steps)? To be clear, the above results were on the production API, and I observed that even by merely clicking the links in your comment above.

Hey @mkevins, I was thinking more along the lines of a network monitoring tool against the prod environment. For example, I was using Charles Proxy but any monitoring tool would probably work.

I'm really hoping to confirm if all of the inconsistencies are network-related or not. While I tested I saw that a particular request would return the result set "A", then on a second call it might return a new result set of "B", then on a subsequent call, I might get "A" again.

So while running the app I might see a sequence like:

  1. query=te returns result "A"
  2. query=newString returns result "C"
  3. query=te returns result "B"
  4. query=newString returns result "C"
  5. query=te returns result "A"

But what I noticed in the app is the display would work as expected and return the results of the last request. So in my example above I would see the results "A". I was wondering if you're seeing the same or not.

I think it makes sense to move forward, and resolve the query issues separately.

👍 I'll go ahead and quote the reply above and put it on the issue #15378 so that we continue the investigation there

@chipsnyder chipsnyder merged commit 546c978 into gutenberg/hpp/2727-enableHPP Nov 24, 2020
@chipsnyder chipsnyder deleted the gutenberg/hpp/bugFixes branch November 24, 2020 12:18
chipsnyder pushed a commit that referenced this pull request Nov 24, 2020
* Enable Home Page Picker feature

* Adjust site creation tests for enabling Home Page Picker

* Resolve small issue where scrollable content was off by 1 pixel

* Make slight layout change to avoid awkward wrapping

* [Home Page Picker] Bug fixes (#15360)

* Prevent swipe to dismiss on site creation flow

* Fix layout constraint collisions

* Resolve issue where the header will sometimes jump around

* Correct issue where it's possible to scroll too far offscreen

* Bump gutenberg version

* Updated the site design prompt wording

* Add the WP custom user agent to site creation webviews (#15376)

* Add Template to Preview analytics (#15365)

* Resolves issue where the selection remains but the results update

* Update Release notes
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.

3 participants