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

feat(tiptap): Loom embed extension #8612

Merged
merged 4 commits into from
Aug 21, 2023
Merged

Conversation

jmtaber129
Copy link
Contributor

@jmtaber129 jmtaber129 commented Aug 2, 2023

Description

Fixes #8659

Implement a read-only Loom extension - when loom links are present in a standup response, add a Loom TipTap node immediately after the paragraph containing the link.

Because this is read-only, other users will always see the expanded Loom embeds, while the author will only see them in closed meetings and the discussion drawer.

Do nothing for summaries, though we could leverage Loom gif thumbnails if we wanted to.

Demo

What the author sees:
Screen Shot 2023-08-15 at 1 12 15 PM

What the reader sees:
Screen Shot 2023-08-15 at 1 12 08 PM

Discussion drawer (both author and readers see this):
Screen Shot 2023-08-15 at 1 15 38 PM

Testing scenarios

  • Write a response with non-loom and loom links, and check the link expansion in various scenarios (bullets, quotes, etc.)
  • Smoke test responses.

Final checklist

  • I checked the code review guidelines
  • I have added Metrics Representative as reviewer(s) if my PR invovles metrics/data/analytics related changes
  • I have performed a self-review of my code, the same way I'd do it for any other team member
  • I have tested all cases I listed in the testing scenarios and I haven't found any issues or regressions
  • Whenever I took a non-obvious choice I added a comment explaining why I did it this way
  • I added the label One Review Required if the PR introduces only minor changes, does not contain any architectural changes or does not introduce any new patterns and I think one review is sufficient'
  • PR title is human readable and could be used in changelog

@github-actions github-actions bot added the size/s label Aug 2, 2023
@jmtaber129 jmtaber129 force-pushed the jmtaber129/tiptap-loom-embeds branch 2 times, most recently from 1e5bdb6 to d6364a4 Compare August 9, 2023 17:28
@jmtaber129 jmtaber129 force-pushed the jmtaber129/tiptap-loom-embeds branch from d6364a4 to 8a9a48e Compare August 11, 2023 19:33
@github-actions github-actions bot added size/m and removed size/s labels Aug 15, 2023
@jmtaber129 jmtaber129 force-pushed the jmtaber129/tiptap-loom-embeds branch from 9cf6f42 to 093b0b4 Compare August 15, 2023 17:55
@jmtaber129 jmtaber129 force-pushed the jmtaber129/tiptap-loom-embeds branch from 093b0b4 to 7073156 Compare August 15, 2023 18:11
@jmtaber129 jmtaber129 changed the title WIP feat(tiptap): Loom embed extension feat(tiptap): Loom embed extension Aug 15, 2023
@jmtaber129 jmtaber129 marked this pull request as ready for review August 15, 2023 18:23
}

const getLoomNodesForParagraph = (content: JSONContent) => {
const distinctLoomLinks: Record<string, string> = {}
Copy link
Member

Choose a reason for hiding this comment

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

+1 since this is a local var, if i hit enter to create a new paragraph & paste the same link again, it's going to unfurl the video again. is this intended?

fwiw i saw that slack messages will do a big unfurl for 1 link, 2 small unfurls for 2 links, and they won't unfurl for 3 links. thought that was kinda cool to prevent it from getting too long! (e.g. thumbnail + title for 1, just thumbnail for 2, just links for 3+). i saw loom has the /oembed path for metadata like title/description if you feel like playing with it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is intended, at least for now - I'd like to get some usage before implementing a more complex unfurling strategy, especially since we don't have a good since for what users might like/dislike for our specific editor. The open question to me is "why might a user include a link to the same loom twice in different paragraphs?" - the answer to this would influence whether we should/shouldn't include a distinct loom embed more than once.

Loom's chrome extension (which embeds loom recordings in GH PR descriptions) also has this duplicate embed behavior, so I don't foresee any significant UX frustrations as a result of this. See screenshot:
Screen Shot 2023-08-21 at 2 06 39 PM

import {Node} from '@tiptap/core'
import {JSONContent, mergeAttributes} from '@tiptap/react'

const LOOM_REGEX = /^(?:https:\/\/)?(?:www\.)?loom\.com\/share\/[a-zA-Z0-9]*\?.*$/g
Copy link
Member

Choose a reason for hiding this comment

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

-1 i don't know how i managed to get an insecure http link while copying & pasting across task cards, but somehow i lost the s & it didn't unfurl. can we use https? instead?

Alternatively, I found the loom official regex, which includes their staging URLs 🤣 . Could use that instead https://www.unpkg.com/browse/@loomhq/loom-embed@1.2.4/dist/esm/common.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ooo yes, nice catch!

Including the staging URLs doesn't feel quite right to me 😂 but I made some minor modifications to our regex based on this, so nice find!

@jmtaber129 jmtaber129 merged commit e1e6958 into master Aug 21, 2023
3 checks passed
@jmtaber129 jmtaber129 deleted the jmtaber129/tiptap-loom-embeds branch August 21, 2023 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TipTap: Loom extension
3 participants