Avoid showing a path conflict when pulling a remote site multiple times - #2239
Conversation
epeicher
left a comment
epeicher
left a comment
There was a problem hiding this comment.
Thanks @sejas for solving this! I have had that issue in the past, and I thought that receiving an error was not a great UX, so I like this!
I have tested it, and it works as expected, I can see the sites with the number added and increasing. I have left a couple of minor comments, but this LGTM!
| Existing site | Selecting the same site | Suffix added | Suffix increased for subsequent sites |
|---|---|---|---|
 |
 |
 |
 |
| <PullRemoteSite | ||
| selectedRemoteSite={ selectedRemoteSite } | ||
| setSelectedRemoteSite={ ( remoteSite?: SyncSite ) => { | ||
| setSelectedRemoteSite={ async ( remoteSite?: SyncSite ) => { |
There was a problem hiding this comment.
I think we can remove the async keyword from here
| const handlePullRemoteContinue = useCallback( async () => { | ||
| if ( selectedRemoteSite ) { | ||
| const availableName = await findAvailableSiteName( selectedRemoteSite.name ); | ||
| void createSiteProps.handleSiteNameChange( availableName ); |
There was a problem hiding this comment.
Now that we have moved this createSiteProps.handleSiteNameChange( availableName ) to an async function, maybe we can get rid of the void operator and replace it for an await? I think we should minimize the use of void to very specific use cases. But this is my personal opinion, happy to leave it as is, as it was the previous behavior.
📊 Performance Test ResultsComparing 936222c vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
katinthehatsite
left a comment
katinthehatsite
left a comment
There was a problem hiding this comment.
The changes look good to me, I am not seeing any regressions and the code reads well as well 👍
3 participants
Related issues
Proposed Changes
Testing Instructions
npm startpull-site-twice.mp4
Pre-merge Checklist