-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Onboarding: Don't use site title as default domain query if it'll just error #48031
Onboarding: Don't use site title as default domain query if it'll just error #48031
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~75 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
This is testing nicely for me for the domain search. I didn't manage to test it out in the created site's launch flow, because of I believe a different issue where using a Japanese site title caused creating the site to fail (I was attempting to create a site with the title "私のサイトのタイトル"). In this case, I left the site title empty, and the I don't have exact testing steps for recreating this, but I tried entering my site title, going to the domain step, entering a search, then clearing the search out completely and skipping the domain step, and then proceeding to attempt to create the site. I think this line is the culprit as it's falling back to the siteTitle for the domain: |
That looks about right, reproduced in Dug a little bit, the underlying error is Turns out we just ignore anything non english and act like nothing was given 👍
Probably best to fix in the endpoint and provide some default text to the create step in this case. We can also try to translit any accented ascii. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, added todo on #37665 for the failure mode @andrewserong discovered.
Thanks for investigating the failure mode @lsl 👍 |
We've been defaulting the domain query to the site title since it seems like a good place to start when looking for a domain. However the domain API returns an error for queries that only include "non-latin" characters, so if a site title is Japanese then the domain step will be in an error state as soon as the user navigates there.
An example of what the user might see when they navigate to the domain step:
(also working on the incorrect network error here: #47985)
By not defaulting to the site title in this case the user will either:
Changes proposed in this Pull Request
isGoodDefaultDomainQuery
helper functionisGoodDefaultDomainQuery
returns falseTesting instructions
/new
flow and experiment using the flow with and without valid site titles and seeing how the domain picker responds to this.Partial #37665