Skip to content

Studio: Open the site's URL from the Sync connect popup - #799

Merged
katinthehatsite merged 4 commits into
trunkfrom
fix/open-site-url-from-sync-popup
Jan 16, 2025
Merged

Studio: Open the site's URL from the Sync connect popup#799
katinthehatsite merged 4 commits into
trunkfrom
fix/open-site-url-from-sync-popup

Conversation

@katinthehatsite

@katinthehatsite katinthehatsite commented Jan 10, 2025

Copy link
Copy Markdown
Contributor

Related issues

Closes https://github.com/Automattic/dotcom-forge/issues/10080

Proposed Changes

This PR makes the URLs in the sync modal open the site when you click on a specific site URL:

Screenshot 2025-01-10 at 2 21 49 PM

Testing Instructions

  • Pull the changes from this branch
  • Start the app with npm start
  • Navigate to the Sync tab
  • Click on Connect site
  • When the modal opens, try clicking on the URL for one of the sites and confirm that it opens the URL

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?
@katinthehatsite katinthehatsite self-assigned this Jan 10, 2025
? '!text-inherit hover:!text-inherit'
: '!text-a8c-gray-30 hover:!text-a8c-gray-30'
) }
onClick={ () => getIpcApi().openURL( site.url ) }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not add this arrow but perhaps we can consider adding it to indicate to the user that it is a link:

Screenshot 2025-01-10 at 2 20 03 PM

What do you think?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we should stick to this convention to provide consistent UX.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I made updates in 4ab21ae

Let me know what you think!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks.

@katinthehatsite
katinthehatsite requested a review from a team January 10, 2025 19:23

@wojtekn wojtekn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code change looks clear. It works as expected.

? '!text-inherit hover:!text-inherit'
: '!text-a8c-gray-30 hover:!text-a8c-gray-30'
) }
onClick={ () => getIpcApi().openURL( site.url ) }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks.

@wojtekn

wojtekn commented Jan 16, 2025

Copy link
Copy Markdown
Contributor

I've just restarted remaining build.

@katinthehatsite

Copy link
Copy Markdown
Contributor Author

Thanks @wojtekn - I restarted twice yesterday but it was taking forever. It seems like a glitch, everything is passing now!

@katinthehatsite
katinthehatsite merged commit fa48819 into trunk Jan 16, 2025
@katinthehatsite
katinthehatsite deleted the fix/open-site-url-from-sync-popup branch January 16, 2025 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants