Add pasting URLs on selected text as Markdown links #23

steffen · 2021-09-01T11:39:41Z

This pull request adds pasting URLs on selected text as Markdown links.

In order to test this new functionality:

Copy this URL https://docs.github.com or any other URL
Go to https://paste-markdown-steffen-testing.vercel.app/examples
Write something like: You can find the docs here.
Select here
Paste URL with cmd + v

Result: You should now see a Markdown link: [here](https://docs.github.com)

koddsson

This looks good to me. Can we add tests for some of this functionality? Also adding an example on the demo page.

src/paste-markdown-link.ts

src/text.ts

Co-authored-by: Kristján Oddsson <[email protected]>

steffen · 2021-09-01T14:25:49Z

@koddsson Thank you for your quick and great suggestions! 🙇‍♂️

I think I've resolved all your inline suggestions as well as the following:

Can we add tests for some of this functionality?

Do you think this test is sufficient? d4c60b1

Also adding an example on the demo page.

I've added an example on the demo page here: 460e1b6
You can also see it here: https://paste-markdown-steffen-testing.vercel.app/examples

I'm happy and open for any other suggestions. Thank you!

steffen · 2021-09-23T17:36:27Z

As discussed in this PR, I changed the default export into named exports in order to allow us to load the new feature separately for feature flagging.

/cc @Bestra @koddsson

koddsson

This looks generally good to me. I had a couple of questions and suggestions.

This is changing the API so this will be a breaking change.

koddsson · 2021-09-24T09:07:30Z

src/index.ts

+function subscribe(el: HTMLElement): Subscription {
  installTable(el)
-  installLink(el)
+  installImageLink(el)


What do you think about having all the install* functions in here so that consumers that use subscribe will get all the features, but then if they want to conditionally apply features they can import only the installers they plan on using?

@koddsson I've thought about that as well. Do you know if this line is the only place where we consume the library at GitHub?

Since subscribe is now a named export, users of the library have to adjust the usage of the library either way, so yeah, I agree with adding the new feature to subscribe right away. We then only need to adjust this line as well to use the add and remove hooks as I do in the lines below so that the new Link feature is not loaded by default.

@koddsson I just saw your proposal on using the add and remove hooks for all functions. I missed that earlier. I'll go with that approach. 👍 Thank you! 🙇‍♂️

@koddsson I have applied your proposal here: 1c6b0cd Thank you! 🙇‍♂️

koddsson · 2021-09-24T09:09:21Z

src/paste-markdown-link.ts

+
+  if (selectionStart > 1) {
+    const previousChars = textarea.value.substring(selectionStart - 2, selectionStart)
+    return previousChars === ']('


What do you think about checking for HTML links as well? Since markdown generally supports HTML as well, should we check if we're in a <a> tag?

I've thought about some other edge cases as well, for example let's say you have this issue template:

Repository: [URL of repository]

If you select [URL of repository] and paste an URL on top of it, with the new feature it would now convert it to [[URL of repository]](url).

Though, I'm not sure we should account for all edge cases or if users will learn quickly that they need to delete the selected text first and then paste the URL in order to have the URL as is.

Regarding detecting if the selected text is within an <a> tag:
Do you think of the use case where a user would have an -link in their Markdown, such as <a href="https://some-url">Docs</a>, and where they would want to change the href by selecting the https://some-url part and replace it with another URL?

steffen · 2021-09-24T09:41:48Z

This is changing the API so this will be a breaking change.

@koddsson I agree. I've raised the breaking of the API here.
@Bestra had the great idea to keep the default export of subscribe alongside the new named exports. Unfortunately rollup.js did not like that, see here.

@koddsson Do you think including a breaking change for 0.4.x is reasonable? Not sure what version schema paste-markdown follows, for semver the major version would need to be bumped, e.g. 0.3.x -> 1.0.0.

steffen · 2021-09-28T10:59:54Z

@koddsson The CI is green now. (I fixed the running of it here and I fixed the eslint error here.)

Is there anything missing in order to merge this PR and create a new release? Thank you!

steffen · 2021-09-28T17:55:24Z

Friendly bump on this PR @github/ui-frameworks-reviewers as I will be off on Friday and I would love to release and deploy these changes this week. Thank you! 🙇‍♂️

srt32 · 2021-09-28T20:10:42Z

👋 I love making it easier to write markdown! Could you share some more of the motivation here? Users can currently highlight text, click cmd-k and get an empty markdown link that they can then fill in with cmd-v.

I personally would find it surprising that cmd-v would not overwrite the text I've selected (and IMO be annoying). I also wonder about making cmd-v be the default (I think it's not configurable?) for the library as a whole as opposed to a configuration option.

WDYT?

edit - I've since been educated that most of the internet already does this ;). Carry on!

steffen · 2021-09-29T06:50:10Z

edit - I've since been educated that most of the internet already does this ;). Carry on!

@srt32 😂 Fair points! I will definitely detail my motivation behind this in our internal staff-shipping post. In short, I believe once you see selected text automagically converted to a link for the first time, you don't want to go back anymore. 😄 Also, I made the functionality feature-flaggable so that we can first enable it for individuals only and then staff-ship it and gather feedback. Thank you for looking into this PR! 🙇‍♂️❤️

steffen · 2021-09-29T19:00:18Z

👋 What is remaining to get this PR merged? 🙇‍♂️

srt32

I'm cool to merge this. Do we have a plan for the change in versions?

steffen · 2021-09-30T06:56:30Z

I'm cool to merge this.

🙇‍♂️

Do we have a plan for the change in versions?

@srt32 I think that would be best. Not sure what version scheme is followed, but this change is a breaking change as it changes the API.
The reason for that is that we changed the export from a default export to a named export in order to support loading selected features (for feature flagging).

So it's now:

import {subscribe} from '@github/paste-markdown'

instead of

import subscribe from '@github/paste-markdown'

This reminds me that we need to update the README as well, which I will do in a moment. (Done: 6c4781f)

@Bestra had the idea to use a default export and a named export. Though, rollup.js didn't like it. We could maybe configure rollup.js to allow it I guess, but we then though the easiest is to just change the API.

srt32 · 2021-09-30T20:55:55Z

https://github.com/github/paste-markdown/releases/tag/v1.0.2 🎉

steffen · 2021-10-01T09:38:11Z

https://github.com/github/paste-markdown/releases/tag/v1.0.2 🎉

Woohoo, thank you @srt32! 🎉 🙇‍♂️

Add pasting URLs on selected text as Markdown links

da17a94

steffen requested a review from a team as a code owner September 1, 2021 11:39

steffen requested review from dgreif and srt32 September 1, 2021 11:39

koddsson reviewed Sep 1, 2021

View reviewed changes

src/paste-markdown-link.ts Outdated Show resolved Hide resolved

src/paste-markdown-link.ts Outdated Show resolved Hide resolved

src/text.ts Outdated Show resolved Hide resolved

steffen and others added 5 commits September 1, 2021 15:29

Define regular expression inline

8f4b8e2

Co-authored-by: Kristján Oddsson <[email protected]>

Use includes instead of indexOf

834a61c

Co-authored-by: Kristján Oddsson <[email protected]>

Change addNewline to be an option

9be6083

Add an example on the demo page

460e1b6

Add test for turning pasted urls on selected text into markdown links

d4c60b1

steffen added 2 commits September 23, 2021 19:33

Add exporting link feature separately (#1)

ac3e95a

Merge branch 'main' into paste-urls-as-markdown-links

a5d440e

koddsson approved these changes Sep 24, 2021

View reviewed changes

Add link feature to subscribe function

1c6b0cd

steffen mentioned this pull request Sep 27, 2021

Make Build workflow run for PRs from forked repos #25

Merged

steffen added 2 commits September 27, 2021 19:16

Merge branch 'main' into paste-urls-as-markdown-links

a3de35a

Fix CI build fail

7840f7d

srt32 approved these changes Sep 29, 2021

View reviewed changes

Update README.md with new API

6c4781f

srt32 merged commit d206bbf into github:main Sep 30, 2021

steffen deleted the paste-urls-as-markdown-links branch October 1, 2021 09:37

This was referenced Oct 4, 2021

Add "undo" support #28

Merged

Only run linkify for URLs #29

Merged

Add pasting URLs on selected text as Markdown links #23

Add pasting URLs on selected text as Markdown links #23

Uh oh!

Conversation

steffen commented Sep 1, 2021

Uh oh!

koddsson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

steffen commented Sep 1, 2021

Uh oh!

steffen commented Sep 23, 2021

Uh oh!

koddsson left a comment

Choose a reason for hiding this comment

Uh oh!

koddsson Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

steffen Sep 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steffen Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

steffen Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

koddsson Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

steffen Sep 24, 2021

Choose a reason for hiding this comment

Uh oh!

steffen commented Sep 24, 2021

Uh oh!

steffen commented Sep 28, 2021

Uh oh!

steffen commented Sep 28, 2021

Uh oh!

srt32 commented Sep 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steffen commented Sep 29, 2021

Uh oh!

steffen commented Sep 29, 2021

Uh oh!

srt32 left a comment

Choose a reason for hiding this comment

Uh oh!

steffen commented Sep 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srt32 commented Sep 30, 2021

Uh oh!

steffen commented Oct 1, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

steffen Sep 24, 2021 •

edited

Loading

srt32 commented Sep 28, 2021 •

edited

Loading

steffen commented Sep 30, 2021 •

edited

Loading