docs(rich-text-editor): link extension tech spec #953
Conversation
MikaDialpad
requested review from
juliodialpad,
iropolo and
braddialpad
as code owners
|
Thanks for contributing to Dialtone Vue! Please read below for some important info regarding Vue 3 compatibility. Currently we need to maintain two branches in Dialtone Vue, one for Vue 2, one for Vue 3. This means you must create two PRs for every feature change you make. One into Many times the change you have made in Vue 2 will be identical to the change you need to make in Vue 3. To make this easier we have made a script that can copy your changes from this branch to a new branch off of staging-vue3 suffixed with -vue3. run Once the new branch is created, you will need to look at your code to make sure it still makes sense and test that your changes all work in vue 3. If everything is good you can push it and create a PR into It is a required check for every PR to have a corresponding branch called I got "commit SHA is a merge but no -m option was given."This happens if there are merge commits in your branch. It's no problem, you can simply skip them with What if I make more changes to my vue 2 branch after running ./copy_pr_vue3.sh?You can copy these to the existing -vue3 branch by running the script with a git SHA param like so:
where 2a78db7 is the last commit from your branch that was copied to the other branch (all commits after this will be copied) If it's just one or two commits, you may prefer to just manually use What if I get a conflict?It's possible to get a conflict when running |
|
FYI @dbecherdialpad |
|
✔️ Deploy Preview ready! |
braddialpad
added
the
no-visual-test
| needs, so we'll have to build a custom extension instead. It would for example | ||
| only validate the link after a following whitespace character, which would leave | ||
| all the links at the end of a message not linkified. With a custom extension we |
There was a problem hiding this comment.
Seems weird it would work like this.. Why would anyone want to leave a link at the end of a message unlinked??
There was a problem hiding this comment.
I don't think this is totally unheard of, I am pretty used to typing a space after pasting a link to have it show up (the mail app does this too). I suspect this behavior might have to do with how the text is parsed and tokenized for perf reasons, which I am a little concerned about with our implementation.
There was a problem hiding this comment.
Yeah I've definitely seen it being implemented like that in a bunch editors, but I really didn't like the UX of the built in extension at least. It doesn't for example validate the word if you go from dialpadcom -> dialpad.com or if you remove space between a valid link and preceding/following word hey [dialpad.com] -> hey[dialpad.com]. I used Slack as a benchmark here as theirs works really well and it seems to do something similar to what I'm proposing here.
| - It *would* be way simpler to just look at the entire content and linkify it | ||
| instead of the substring, but that could manifest in performance issues in | ||
| extreme cases (super long messages and/or tons of links in the text) and we want | ||
| this to be very quick since it is run after every keystroke. |
There was a problem hiding this comment.
This does seem way simpler, perhaps we should do a performance test and see if it makes any significant difference? no need to micro-optimize unless necessary.
There was a problem hiding this comment.
Nah I definitely think we should do it something like this (not parsing all the text every single change), BUT I am curious about what I alluded to above with marks that span whitespace boundaries. How do we know how far back/forward to look? It can't just be the current word since that wouldn't catch phone numbers with spaces.
There was a problem hiding this comment.
Yeah good points. I commented above that I've seen some lagginess when testing the very extreme cases of super longs texts with tons of links while matching the entire text. For DP messaging that might not be that relevant since we have character limits, but if the agent experience team uses this to compose emails then it could become a real life issue.
For my proposed way the text to match isn't affected by how long the message is, so it doesn't really degrade at all. Also I'm thinking that this is just one extension we're implementing, but eventually we will have a bunch more that might do something similar and they can have a compounding effect. I have a POC for how this could work (barring the phone number edge case) which I can show if you're interested.
| - 7-15 characters long | ||
| - Without separators, e.g. `7144107035` | ||
| - With dashes `-`, e.g. `714-410-7035` | ||
| - With whitespace `\s`, e.g. `714 410 7035` |
There was a problem hiding this comment.
I feel like "with whitespace" might end up causing us grief...just a note that if it ends up being a hassle or performance issue, I would be ok NOT highlighting this format in the editor
There was a problem hiding this comment.
Ugh I saw your other comment about the word separating characters and I think you're spot on there. I haven't actually tried to implement phone numbers yet, so I don't know how easy it would be to overcome this issue, but I'll definitely try before abandoning any formats.
| ### Common Editor Behavior | ||
|
|
||
| - All links are invalid (not linkified) if preceded by `#` or `@` characters, | ||
| since they're reserved for user mentions and channel hashtags. |
There was a problem hiding this comment.
Or: literal numbers (eg #12345)
|
|
||
| - All links are invalid (not linkified) if preceded by `#` or `@` characters, | ||
| since they're reserved for user mentions and channel hashtags. | ||
| - Similarly links wrapped with ` ``` ` will not be linkified as the triple |
There was a problem hiding this comment.
Does this maybe more generally mean "we won't linkify things contained in another node" or are there cases where we might want to?
There was a problem hiding this comment.
Great question! I don't think we need to worry about that too much yet, but I think it'll depend on the node. For codeblocks we most likely don't want any linking or any other formatting like bolding, but if one day we wanted to build a vcard extension for example then we probably want the contents to be linkified.
And now testing this again I see that codeblocks don't just work in my beta at all??? So my initial tests were giving wrong results as I saw something like ```dialpad.com``` shown as plain text and thought the missing whitespace invalidated the codeblock. Anyways, I will not do any special rules for backticks now but eventually when we have node extensions we'll have to probably exclude or include them here.
| substring of the changed content is not enough. For example, if we had content | ||
| `Hey dialpadcom` that was changed to `Hey dialpad.com`, the transaction would | ||
| give us the range for just the changed content, which is the added `.`. What we | ||
| actually need is the entire word, or words, that were affected, so `dialpad.com` |
There was a problem hiding this comment.
Boy this sure seems really complicated. What's the other approach that just uses a regex?
There was a problem hiding this comment.
So this would still use regex, but the string to match with would be shorter than the alternative which is to just match the entire text content of the editor. I did some benchmarking and for short messages the difference is non-existent, but for reeeally long messages typing might start to lag on low spec machines.
I have a POC with this and it doesn't seem that bad except for the phone number edge case that you brought up. I'll try it out to see how much more complex it makes this.
| - It *would* be way simpler to just look at the entire content and linkify it | ||
| instead of the substring, but that could manifest in performance issues in | ||
| extreme cases (super long messages and/or tons of links in the text) and we want | ||
| this to be very quick since it is run after every keystroke. |
There was a problem hiding this comment.
Nah I definitely think we should do it something like this (not parsing all the text every single change), BUT I am curious about what I alluded to above with marks that span whitespace boundaries. How do we know how far back/forward to look? It can't just be the current word since that wouldn't catch phone numbers with spaces.
|
✔️ Deploy Preview ready! |
|
✔️ Deploy Preview ready! |
braddialpad
changed the title
|
✔️ Deploy Preview ready! |
Thanks for the review! Is there some label to bypass the visual_tests task that's blocking the merge? |
|
Ugh sorry I have to fix that.. I'll just force merge for you for now. |
# [2.83.0](v2.82.1...v2.83.0) (2023-05-12) ### Bug Fixes * correct style export in package.json ([f00cb19](f00cb19)) * **Feed Items:** emoji row emits naming fix ([#976](#976)) ([a4c7b86](a4c7b86)) * update dialtone-icons ([983b178](983b178)) * visual test stories showing up in prod ([#968](#968)) ([7135cc2](7135cc2)) ### Documentation * **Rich Text Editor:** link extension tech spec ([#953](#953)) ([d48af5c](d48af5c)) ### Features * **Feed Items:** create feed item emoji row ([#972](#972)) ([265b504](265b504)) * **Size And Space:** apply between stops and negative variables ([#964](#964)) ([d2aab13](d2aab13))
|
🎉 This PR is included in version 2.83.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
docs(rich text editor): link extension tech spec
🛠️ Type Of Change
📖 Description
Tech spec for the Link extension used by DtRichTextEditor. I didn't touch testing yet since I'm not sure how easy it is to test something like this as the bulk of the functionality will be in the ProseMirror plugin. I'll have to try it out and add a section here later on.
For easier reading: https://github.com/dialpad/dialtone-vue/blob/3b0afb5d4e48c20c6621555cc8ea8f9fd4833cc3/components/rich_text_editor/rich_text_editor.tech_spec.md
🔮 Next Steps
Implement the extension and see how we can write tests for it.