Conversation
|
@aforcier I am not sure what the process is like for updating the login library as mentioned on Slack. I thought I'd just open the PR and take it from there. So, I am not touching the lint errors for now. |
mattmiklic
left a comment
There was a problem hiding this comment.
These screenshots look good, thanks for confirming the alignment is right, @oguzkocer.
|
Works well functionally, tested various URL cases 👍 As I shared on Slack, you'll need to run The script automatically adds new strings to the bottom of WordPress' |
|
Thanks for the review @aforcier! I've manually made the changes as it was taking too long to setup my machine for the script. Hopefully it's fixed now! |
|
|
Fixes #8460. This PR improves the error messages for invalid site url. Instead of relying on the FluxC callback to check if the input text is a valid url, we'll first do a pattern check and if it fails, we'll show
login_invalid_site_urlmessage. If the pattern matches a url, we'll let FluxC do it's thing and if finds that there are no sites at that url, it's more likely that it's not a WordPress site, so we show the updatedno_site_errormessage.There are several different ways to check if it's a valid url, but surprisingly out of all the options I tried, only one of them worked exactly how I want this to. Here are a couple issues:
UrlUtils.isValidUrlAndHostNotNull, we'll need to put in the scheme in which case it says it's a valid url when I enterarst(which will be converted tohttp://arst)URLUtil.isValidUrl()suffers from the same issue aboveSee: https://stackoverflow.com/a/5930532
Copy Updates
I've requested two different messages from editorial in an internal p2 post and they got back to me with the following messages:
login_invalid_site_url: The site address you entered is invalid. Please re-enter it.
no_site_error: The site at this address is not a WordPress site. For us to connect to it, the site must use WordPress.
I don't think it's necessary to do another editorial review.
Design Review
I brought up this issue on Slack and per @mattmiklic:
I am not entirely sure why the line break issue was happening, but it seems to be working fine for me. For the spacing, I think it's correct right now, maybe the screenshot in the original issue is a wrong one. Here are screenshots of the updated message with and without the layout bounds:
To test
Test the following cases:
..but with a url scheme such ashttp://Here are my tests: https://cloudup.com/coSPbKhY4lU
Update release notes:
- [ ] If there are user facing changes, I have added an item toRELEASE-NOTES.txt.I don't think this is big enough of a change to have in the release notes, but let me know if you feel otherwise.