Validate custom domain name availability - #1114
Conversation
bcotrim
left a comment
bcotrim
left a comment
There was a problem hiding this comment.
Nice improvement! 👍
I don't like how we need to fetch the userData for each validation. It's probably outside of the scope of this issue, but now that we have Redux, I think it would be great if we could make our userData sync with a Redux store, fixing all issues similar to this one.
I added a comment with an issue I've found. Besides that I think the code is good to go, if we want to tackle the Redux change in the future.
| const [ existingDomainNames, setExistingDomainNames ] = useState< string[] >( [] ); | ||
| const [ enableHttps, setEnableHttps ] = useState( false ); | ||
|
|
||
| useEffect( () => { |
There was a problem hiding this comment.
This is only loading the domains once.
So if you create a site with a custom domain and, without restarting the app, try to create another one using the same domain, it will allow it.
There was a problem hiding this comment.
Good catch, thanks 👍
wojtekn
left a comment
wojtekn
left a comment
There was a problem hiding this comment.
Works as expected. I get correct validation error in both Add site and Edit site forms.
| }; | ||
|
|
||
| export const validateDomainName = ( | ||
| export const getDomainNameValidationError = ( |
There was a problem hiding this comment.
Nice rename and validation improvements.
Agreed 👍 We could probably remove a good chunk of the IPC methods if we did this and reduce the overall complexity. But yeah, it's definitely outside the scope of this PR |
bcotrim
left a comment
bcotrim
left a comment
There was a problem hiding this comment.
Tested and confirmed the issue no longer happens, thanks for making the change!
LGTM 👍
| .catch( () => { | ||
| // Do nothing | ||
| } ); | ||
| }, [ selectedSite?.customDomain ] ); |
There was a problem hiding this comment.
It would be cool to extract this code into a useAvailableCustomDomains() hook to improve readability.
4 participants
Related issues
Proposed Changes
Testing Instructions
Pre-merge Checklist