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

Add link to coaching notes editor #98

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

zgavin1
Copy link
Contributor

@zgavin1 zgavin1 commented Apr 3, 2025

Description

Editor feature for adding/removing links.

A user may now in the coaching notes editor incorporate hyperlink(s) into the editor content.

The toolbar button should open a dialog box with a field for the url.

Large part of this logic borrowed from TipTap's example

Link text should be highlighted a standard blue.

Addresses: https://docs.google.com/document/d/10gTCL0uNu1VHEFQl_PzPmnn07QOcDw89ta5C-UVh9vQ/edit?tab=t.0#heading=h.r4v5w0aeykos

Changes

  • Install and add link extension to extensions groups
  • Add toolbar button for link to toolbar
  • Add link dialog component
  • Logic for inserting/removing link

Screenshots / Videos Showing UI Changes (if applicable)

Screenshot 2025-04-03 at 11 37 11 AM Screenshot 2025-04-03 at 11 37 08 AM

Testing Strategy

  • Navigate to a coaching session
  • Click link button in toolbar
  • Insert a link
  • Insert another link
  • Remove a link
  • Update an existing link

Concerns

@jhodapp We may want to review some specifics with how you want this to work - validating links, disallowing some domains, etc. Will leave this in draft until we chat about that

@zgavin1 zgavin1 requested a review from jhodapp April 3, 2025 16:42
@jhodapp jhodapp added enhancement Improves existing functionality or feature feature work Specifically implementing a new feature labels Apr 3, 2025
@jhodapp jhodapp added this to the 1.0-beta1 milestone Apr 3, 2025
Copy link
Member

@jhodapp jhodapp left a comment

Choose a reason for hiding this comment

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

Looking great Zach, thank you for this. A few suggestions inline.

How difficult would it be to allow for updating an existing link and then following the link if one presses ?

: new URL(`https://${url}`);

// only auto-link if the domain is not in the disallowed list
const disallowedDomains = [
Copy link
Member

Choose a reason for hiding this comment

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

@zgavin1 I think a good central place for this list right now is to place this in site.config.ts under siteConfig { tiptap { links { } } }

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 can also just comment this out.. Do we even want these?

Copy link
Member

Choose a reason for hiding this comment

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

I think so, it's probably better to have it than not. No immediate use but I'm guessing it's a pattern for TipTap for a good reason.

<DialogHeader>
<DialogTitle>Insert Link</DialogTitle>
<DialogDescription>
Insert a url for a link in your document.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Insert a url for a link in your document.
Insert URL for your link.

Comment on lines 147 to 150
a {
color: #0000EE;
text-decoration: underline;
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this still needed?

onClick={() => setIsLinkDialogOpen(true)}
isActive={editor.isActive("link")}
icon={<Link className="h-4 w-4" />}
title="Link"
Copy link
Member

Choose a reason for hiding this comment

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

@zgavin1 Can you dynamically have this text change like so:

  • "Insert Link (Ctrl+K)" for when inserting a link
  • "Update Link (Ctrl+K)" when one has highlighted a link?

@jhodapp jhodapp moved this from Review to 🏗 In progress in Refactor Coaching Platform Apr 3, 2025
@zgavin1 zgavin1 requested a review from jhodapp April 8, 2025 01:22
@zgavin1 zgavin1 marked this pull request as ready for review April 8, 2025 01:22
@jhodapp jhodapp moved this from 🏗 In progress to Review in Refactor Coaching Platform Apr 8, 2025
Copy link
Member

@jhodapp jhodapp left a comment

Choose a reason for hiding this comment

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

A few suggestions inline, looking great Zach.

Right click to open link doesn't work for me, but perhaps you can learn from this solution to give SHIFT + left click another try.

Comment on lines +122 to +123
? "Update link (Ctrl + k)"
: "Insert link (Ctrl + k)"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
? "Update link (Ctrl + k)"
: "Insert link (Ctrl + k)"
? "Update Link (Ctrl + k)"
: "Insert Link (Ctrl + k)"

This makes it consistent with the other buttons' tooltip capitalization.

// Add keyboard shortcut handler
useEffect(() => {
const handleKeyDown = (e: KeyboardEvent) => {
if ((e.metaKey || e.ctrlKey) && e.key === "k") {
Copy link
Member

Choose a reason for hiding this comment

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

This isn't working today because of our command menu at the top, it listens for CMD + k as well which is in conflict. We do want to hide/remove that search for the beta anyway, so if you do that, this should work.

I changed the key combination to listen for 'l' and then CTRL + l worked for me.

Comment on lines +39 to +65
// Placeholder for future disallowed domains if we want to add any
// const disallowedProtocols = ["ftp", "file", "mailto"];
// const protocol = parsedUrl.protocol.replace(":", "");

// if (disallowedProtocols.includes(protocol)) {
// return false;
// }

// // only allow protocols specified in ctx.protocols
// const allowedProtocols = ctx.protocols.map((p) =>
// typeof p === "string" ? p : p.scheme
// );

// if (!allowedProtocols.includes(protocol)) {
// return false;
// }

// Placeholder for future disallowed domains if we want to add any
// const disallowedDomains = [
// "example-phishing.com",
// "malicious-site.net",
// ];
// const domain = parsedUrl.hostname;

// if (disallowedDomains.includes(domain)) {
// return false;
// }
Copy link
Member

Choose a reason for hiding this comment

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

Is this still needed? You have implemented some or all of this already from what I can tell.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improves existing functionality or feature feature work Specifically implementing a new feature
Projects
Status: Review
Development

Successfully merging this pull request may close these issues.

2 participants