Skip to content

Update demo sites to open link in browser instead of copying the link #790

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.

Sign up for GitHub

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

Jump to bottom
Merged
sejas merged 2 commits into from
Jan 9, 2025
Merged

Update demo sites to open link in browser instead of copying the link #790

sejas merged 2 commits into from
Jan 9, 2025

Conversation

@sejas
Copy link
Member

@sejas sejas commented Jan 8, 2025

Related issues

Proposed Changes

  • Replace link in demo sites to open the browser instead of copy the link.
  • Add tests to check the link opens the browser with the correct link

It follows the same style as Sync links but without tooltip.

studio/src/components/sync-connected-sites.tsx

Lines 225 to 235 in 8176093

<Tooltip text={ connectedSite.url } className="overflow-hidden">
<Button
variant="link"
className="!text-a8c-gray-70 hover:!text-a8c-blueberry max-w-[100%]"
onClick={ () => {
getIpcApi().openURL( connectedSite.url );
} }
>
<span className="truncate">{ connectedSite.url }</span> <ArrowIcon />
</Button>
</Tooltip>

Testing Instructions

  • Run `npm start``
  • Click on Share tab
  • Create a new demo site
  • Observe the demo site link opens the browser instead of copying it
  • Tests should pass
Before After
Screenshot 2025-01-08 at 20 42 16 Screenshot 2025-01-08 at 20 42 08

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@sejas sejas self-assigned this Jan 8, 2025
@sejas sejas requested a review from a team January 8, 2025 20:50
fredrikekelund
fredrikekelund approved these changes Jan 9, 2025
View reviewed changes
Copy link
Contributor

@fredrikekelund fredrikekelund left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Why did we add the tooltip in the Sync tab again..? There's less horizontal space in the Sync tab compared to the Share tab. Is that it?

@sejas
Copy link
Member Author

sejas commented Jan 9, 2025

LGTM 👍 Why did we add the tooltip in the Sync tab again..? There's less horizontal space in the Sync tab compared to the Share tab. Is that it?

Yes! That’s the reason. We have less space in the Sync tab, and the domains are determined by the user, which means they can be very long and might be truncated. In contrast, the subdomains generated in the Share tab are a combination of 2-3 words.

@sejas sejas merged commit a8447ba into trunk Jan 9, 2025
6 checks passed
@sejas sejas deleted the update/demo-sites-open-link-in-browser branch January 9, 2025 13:03
@wojtekn
Copy link
Contributor

wojtekn commented Jan 9, 2025

Thanks for the fix, it improves the UX around demo sites.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fredrikekelund fredrikekelund fredrikekelund approved these changes

Assignees

@sejas sejas

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sejas @wojtekn @fredrikekelund