Add architecture for granual pasteAsPlainText support #68

anleac · 2022-09-15T18:10:51Z

Motive

We wish to be able to have granual control over specifically the behaviour of whether to linkify when pasting a link over selected text. This pull request extends the current code base to support granual config support on the installation of the package, at this time, it only adds one control being for the paste-markdown-link (urlLinks in our API params).

This config, which is default set to false (undefined | false), is the existing behaviour and therefore not a breaking change.

The approach would therefore add the following "new" behaviour, given the pasteLinkAsPlainTextOverSelectedText flag for paste-markdown-link, and depending on whether the skipFormattingFlag is on/off:

paste plain text is on, no skip flag -> paste as plain
paste plain text is on, skip flag -> try linkify
paste plain text is off, no skip flag -> try linkify
paste plain text is off, skip flag -> paste as plain

As we add future support, we should extend this down to the future classes + add tests, I do not believe it would be much extra work but let's do it as a follow up or as a per-need basis.

Drawbacks

End-users may see this optional config as misleading, as we are passing in the options params to all base paste classes but only have it supported in one currently, but the documentation will reflect this.

mboisvertdupras · 2022-09-15T18:16:53Z

Looks good! 👍

theinterned

There's some fragility to storing state on DOM nodes. Did you consider a weak map of element to value?

src/index.ts

anleac · 2022-09-16T18:44:29Z

There's some fragility to storing state on DOM nodes. Did you consider a weak map of element to value?

Indeed, I considered having a weak map inside the paste-markdown-link for element -> boolean mapping. I went back/fourth on this approach just due to having to extend the install functions on each paste-markdown-* file (due to the installation pattern here: https://github.com/github/paste-markdown/blob/main/src/paste-keyboard-shortcut-helper.ts#L27

That being said, I am happy to update to this if you believe this is a stronger approach 👍

theinterned · 2022-09-16T19:03:21Z

can you explain the difference between shouldPastePlainText and shouldSkipFormatting

anleac · 2022-09-16T19:08:33Z

can you explain the difference between shouldPastePlainText and shouldSkipFormatting

The difference is that, even in the event of shouldPastePlainText being true, we would still like the option to "skip formatting" of that value, which would shift it to linkifying.

anleac · 2022-09-16T19:10:45Z

There's some fragility to storing state on DOM nodes. Did you consider a weak map of element to value?

I also just pushed a commit here cd58a52 to use a WeakMap instead, and updated the description

theinterned · 2022-09-16T19:13:13Z

we would still like the option to "skip formatting" of that value, which would shift it to linkifying.

@anleac – so this would allow for a config that does not apply other formatting like images, tables, bold etc. but DOES linkify still?

Another way to do that is just to subscribe to a subset of the functionality.

- const unsubscribe = subscribe(el)
+ installLink()

anleac · 2022-09-16T19:19:46Z

we would still like the option to "skip formatting" of that value, which would shift it to linkifying.

@anleac – so this would allow for a config that does not apply other formatting like images, tables, bold etc. but DOES linkify still?

Yes that was the current motive - it would be similar to the toggle switch Edge has here (it is not the same, but similar) in being able to toggle rich HTML or plain text by default

anleac · 2022-09-20T11:38:08Z

My proposal, which I updated in 2661306, we can make the config name very specific (pasteLinkAsPlainTextOverSelectedText) to try to mitigate end user confusion, and we can update to add more modular control afterwards if needed.

theinterned

I really like where you've landed on this. It has room to scale to other formatters if needed. Note.

I do have the one API question – I think it's worth keeping the signature of all install functions parallel.

I would also like to see some docs added to the readme about this.

theinterned · 2022-09-20T19:27:50Z

src/option-config.ts

+}
+
+interface PlainTextParams {
+  urlLinks?: boolean


theinterned · 2022-09-20T19:31:03Z

src/paste-keyboard-shortcut-helper.ts

+  installCallbacks: Array<(el: HTMLElement) => void>,
+  installCallbacksWithOptions: Array<(el: HTMLElement, optionConfig?: OptionConfig) => void>,


rather than have two arrays here, why not just pass options to all the callbacks?

I was hoping to avoid confusion given we export the individual installation functions, too, but the documentation update would be sufficient. Let me tackle this now

Infact I am not even sure this is needed, I believe still keeping them to the same array and simplying defaulting to passing in config params in all cases works (tests are still passing).

If my understanding is correct, it'll simply ignore the param

anleac · 2022-09-20T19:41:52Z

I really like where you've landed on this. It has room to scale to other formatters if needed. Note.

I do have the one API question – I think it's worth keeping the signature of all install functions parallel.

I would also like to see some docs added to the readme about this.

👋 my concern with sharing the installation signature across all the functions is that only one actually can be influenced by the config, while the rest wouldn't be, and we export all of these installations which possibly could be misleading. I guess we can circumvent this via the docs and marking this as not yet implemented?

theinterned · 2022-09-20T20:10:10Z

I guess we can circumvent this via the docs and marking this as not yet implemented?

yeah ... that was my thought. The actual type signature for the Options should hopefully make it clear what the options are.

I guess the other option would be to have the install commands implement either an Install or InstallWithOptions type and then do type narrowing of some kind in installAround.

theinterned

This is looks great! Thank you for being patient and working through the multiple rounds of feedback. I think the extra work should pay off in keeping this API healthy.

theinterned · 2022-09-21T14:36:58Z

README.md

+Only the `urlLinks` param is currently supported.
+
+If there is no config passed in, or attributes missing, this will always default to `false`, being the existing behavior.
+


thank you this is great 👏

Support paste config

c8cab78

anleac requested a review from a team as a code owner September 15, 2022 18:10

anleac requested a review from theinterned September 15, 2022 18:10

theinterned reviewed Sep 16, 2022

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

src/index.ts Outdated Show resolved Hide resolved

Remove debug statements

bd9a7fb

Update to use weak map

cd58a52

Make config flag more specific

2661306

anleac changed the title ~~Support config param for shouldPastePlainText~~ Support config param for pasteLinkAsPlainTextOverSelectedText Sep 20, 2022

PoC for granual support

9bd8558

theinterned reviewed Sep 20, 2022

View reviewed changes

anleac changed the title ~~Support config param for pasteLinkAsPlainTextOverSelectedText~~ Add architecture for granual pasteAsPlainText support Sep 21, 2022

Update readme

c81e20b

theinterned approved these changes Sep 21, 2022

View reviewed changes

manuelpuyol merged commit 029c5e4 into github:main Sep 21, 2022

iansan5653 mentioned this pull request Sep 23, 2022

Add a prop to disable link pasting in MarkdownEditor primer/react#2363

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add architecture for granual pasteAsPlainText support #68

Add architecture for granual pasteAsPlainText support #68

anleac commented Sep 15, 2022 •

edited

Loading

mboisvertdupras commented Sep 15, 2022

theinterned left a comment

anleac commented Sep 16, 2022

theinterned commented Sep 16, 2022

anleac commented Sep 16, 2022

anleac commented Sep 16, 2022

theinterned commented Sep 16, 2022 •

edited

Loading

anleac commented Sep 16, 2022

anleac commented Sep 20, 2022 •

edited

Loading

theinterned left a comment •

edited

Loading

theinterned Sep 20, 2022

theinterned Sep 20, 2022

anleac Sep 21, 2022

anleac Sep 21, 2022

anleac commented Sep 20, 2022

theinterned commented Sep 20, 2022

theinterned left a comment

theinterned Sep 21, 2022

		installCallbacks: Array<(el: HTMLElement) => void>,
		installCallbacksWithOptions: Array<(el: HTMLElement, optionConfig?: OptionConfig) => void>,

		Only the `urlLinks` param is currently supported.

		If there is no config passed in, or attributes missing, this will always default to `false`, being the existing behavior.

Add architecture for granual pasteAsPlainText support #68

Add architecture for granual pasteAsPlainText support #68

Conversation

anleac commented Sep 15, 2022 • edited Loading

Motive

Drawbacks

mboisvertdupras commented Sep 15, 2022

theinterned left a comment

Choose a reason for hiding this comment

anleac commented Sep 16, 2022

theinterned commented Sep 16, 2022

anleac commented Sep 16, 2022

anleac commented Sep 16, 2022

theinterned commented Sep 16, 2022 • edited Loading

anleac commented Sep 16, 2022

anleac commented Sep 20, 2022 • edited Loading

theinterned left a comment • edited Loading

Choose a reason for hiding this comment

theinterned Sep 20, 2022

Choose a reason for hiding this comment

theinterned Sep 20, 2022

Choose a reason for hiding this comment

anleac Sep 21, 2022

Choose a reason for hiding this comment

anleac Sep 21, 2022

Choose a reason for hiding this comment

anleac commented Sep 20, 2022

theinterned commented Sep 20, 2022

theinterned left a comment

Choose a reason for hiding this comment

theinterned Sep 21, 2022

Choose a reason for hiding this comment

anleac commented Sep 15, 2022 •

edited

Loading

theinterned commented Sep 16, 2022 •

edited

Loading

anleac commented Sep 20, 2022 •

edited

Loading

theinterned left a comment •

edited

Loading