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

Site Creation Visual Cue for Unavailable or Invalid Domains #10717

Merged
merged 27 commits into from Dec 9, 2019

Conversation

@jd-alexander
Copy link
Contributor

jd-alexander commented Oct 31, 2019

Fixes #10089
Now when the user navigates to the 3rd step of the Site Creation process and they are selecting their domain they will be notified if the query they entered is already in use or invalid. Previously, only suggestions would have been shown.

To test:

  1. My Site.
  2. Switch site.
  3. Press the add button.
  4. Create a new WordPress.com site.
  5. Skip to the screen where you can search for a domain and begin utilizing different search queries to see the behavior.

Below is a gif of this in action.

Before on the left and the after on the right.

Todo

  • Do accessibility audit on the layout and make changes to support it.
  • Verify the domain cases that the app needs to validate.

PR submission checklist:

  • I have considered adding unit tests where possible.

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

This involves checking the current domain against the domain suggestions and seeing if it's actual available and creating state that represents that.
… result that will help to create the Domain List state.

Also refactored the way how the state was being created and created means by which an invalid result from the validator can be shown on the screen.
@jd-alexander jd-alexander requested a review from malinajirka Oct 31, 2019
@jd-alexander jd-alexander added this to the 13.6 milestone Oct 31, 2019
@jd-alexander jd-alexander force-pushed the issue/10089/site-creation-visual-cue branch from 126cc91 to b42f7dd Oct 31, 2019
@jd-alexander jd-alexander force-pushed the issue/10089/site-creation-visual-cue branch from b42f7dd to 05ab727 Oct 31, 2019
@jd-alexander

This comment has been minimized.

Copy link
Contributor Author

jd-alexander commented Oct 31, 2019

I will fix the build in the morning. Need to update Gutenberg it seems.

@peril-wordpress-mobile

This comment has been minimized.

Copy link

peril-wordpress-mobile bot commented Oct 31, 2019

You can test the changes on this Pull Request by downloading the APK here.

…e ViewModel test suite to verify the new state is being added when domain result varies.
@jd-alexander jd-alexander force-pushed the issue/10089/site-creation-visual-cue branch from 8d3125e to f466e8f Oct 31, 2019
Copy link
Contributor

malinajirka left a comment

Thank you @jd-alexander, great job! This whole task is way more complicated than I anticipated. Seeing all the changes, having the logic on the server side would save us a lot. Have you asked someone on the API if they could implement it for us? I'm just worried that we won't be able to mirror the behavior on iOS as we rely on some internal classes.

Thanks for the inline comments with question/explanations! One thing which made the PR a bit more difficult to review are white-space/line-break changes -> they make the change set unnecessary bigger. It'd be great to make sure not to commit these in other PRs unless it increases readability. Thanks!

I haven't tested the app as I left some comments which might result in changes. Let me know what you think about my suggestions. Please don't just implement what I suggested and let me know your opinion ;). Thanks!

@jd-alexander

This comment has been minimized.

Copy link
Contributor Author

jd-alexander commented Nov 1, 2019

A question to those who have worked on the API. So here we are working on an issue in Site Creation. In particular, the domain selection stage where a user can enter a query and they get domain suggestions (“/domains/suggestions”). Currently, the app only shows the suggestions but it doesn’t state if the domain query that was entered is available or not or if it was invalid altogether so we are working on adding that. At first we were thinking to add the logic on the client-side due to not wanting to make two server-side requests p1571736509104000-slack-CFF433YSG; that is one to /domains/suggestions and another to /domains/$domain/is-available but now after trying to do the domain verification logic on the client we want more feedback if it worth doing this on the client with two seperate requests or if both of these requests could be combined into one?

@jkmassel

This comment has been minimized.

Copy link
Contributor

jkmassel commented Nov 4, 2019

Howdy folks! I'm freezing 13.6 today, so this is being bumped to 13.7. If you'd like it to go out as part of 13.6, please target release/13.6 for this PR, and DM me once it's merged – I'll be happy to cut a new beta release!

@jkmassel jkmassel modified the milestones: 13.6, 13.7 Nov 4, 2019
@jkmassel

This comment has been minimized.

Copy link
Contributor

jkmassel commented Nov 18, 2019

Howdy folks! I'm freezing 13.7 today, so this is being bumped to 13.8. If you'd like it to go out as part of 13.7, please target release/13.7 for this PR, and DM me once it's merged – I'll be happy to cut a new beta release!

jd-alexander added 10 commits Nov 28, 2019
This function was added to aid the support of the domain validation that was being done.
The site creation viewmodel that shows domain suggestions is now sanitizing the query being passed before determining if the domain exists in the suggestions or not. It's UI state is also being tested with the unavailability state to ensure it behaves correctly.
@jd-alexander jd-alexander requested a review from malinajirka Nov 29, 2019
@jd-alexander

This comment has been minimized.

Copy link
Contributor Author

jd-alexander commented Nov 29, 2019

Hi @malinajirka I responded to all the comments you made about the previous implementation and cross-referenced them with the current implementation that follows the algorithm you came up with it. Let me know what you think after your review. Happy to answer any questions you might have 🙌 Again, thanks for the thorough reviews you have been providing. They have been really helpful 🙏

Copy link
Contributor

malinajirka left a comment

Really great job @jd-alexander! The code looks way easier to follow now. I have found one issue regarding Locale, but it looks really good overall. I haven't tested the app yet as it's not buildable. Let me know what you think about my minor suggestions and please ping me when the build passes. Thank you!

Double-check:
I've never had to do Submodule gutenberg-mobile updated from 6090fd to 741109. Did you check with the gutenberg team that it's really required?

@jkmassel

This comment has been minimized.

Copy link
Contributor

jkmassel commented Dec 3, 2019

We're freezing 13.8 today, so I'm moving this forward to 13.9. If you'd like it released as part of 13.8, please adjust the target from develop to release/13.8, and let me know once this PR has been merged – I'll be happy to ship a new release candidate!

@jkmassel jkmassel modified the milestones: 13.8, 13.9 Dec 3, 2019
@jd-alexander jd-alexander force-pushed the issue/10089/site-creation-visual-cue branch from 78d479c to ae1da2d Dec 3, 2019
@jd-alexander jd-alexander requested a review from malinajirka Dec 4, 2019
Copy link
Contributor

malinajirka left a comment

Thanks @jd-alexander! I've tested the app and everything looks great! Thank you for all the changes and I'm sorry for all the complication you encountered in this issue. I'm really glad both the code and the UX look great now ;)!

SideNote: I haven't tested TalkBack support but I'd suggest working on it in a different issue/PR anyway.

@malinajirka malinajirka merged commit 2974e01 into develop Dec 9, 2019
6 checks passed
6 checks passed
Peril All green. Jolly good show.
Details
ci/circleci: Installable Build Your tests passed on CircleCI!
Details
ci/circleci: connected-tests Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: strings-check Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
@malinajirka malinajirka deleted the issue/10089/site-creation-visual-cue branch Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.