Skip to content
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

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

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

katinthehatsite
Copy link
Contributor

@katinthehatsite katinthehatsite commented Jan 10, 2025

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
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
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
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
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
Copy link
Contributor

@wojtekn wojtekn left a comment

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
Contributor

Choose a reason for hiding this comment

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

Looks great, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants