fix: Markdown editor unregister tooltip plugin #4383

K-Kumar-01 · 2020-12-15T10:42:32Z

Issue number #4239
Ability to unregister the tooltip plugin added
Updated the editor example doc (mardowm_editor_example.js) to show how to use it

Summary

Provide a detailed summary of your PR. Explain how you arrived at your solution. If it includes changes to UI elements include a screenshot or gif.

Checklist

Check against all themes for compatibility in both light and dark modes
Checked in mobile
Checked in Chrome, Safari, Edge, and Firefox
- [ ] Props have proper autodocs
~~Added~~ Updated documentation
~~- [ ] Checked Code Sandbox works for the any docs examples~~
- [ ] Added or updated jest tests
Checked for breaking changes and labeled appropriately
~~- [ ] Checked for accessibility including keyboard-only and screenreader modes~~
A changelog entry exists and is marked appropriately

Issue number 4239 Ability to unregister the tooltip plugin added Updated the editor example doc to show how to use it

cla-checker-service · 2020-12-15T10:42:35Z

💚 CLA has been signed

kibanamachine · 2020-12-15T10:42:36Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

elizabetdev · 2020-12-15T14:16:51Z

Hi @K-Kumar-01, thanks for contributing to EUI!

Can you please sign the Contributor agreement using the email associated with your GitHub account?

elizabetdev · 2020-12-15T14:17:01Z

jenkins test this

kibanamachine · 2020-12-15T14:37:42Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4383/

K-Kumar-01 · 2020-12-16T09:53:48Z

Hi @K-Kumar-01, thanks for contributing to EUI!

Can you please sign the Contributor agreement using the email associated with your GitHub account?

In the Title field, I have put my name for now, as I didn't know what to put there.

chandlerprall · 2020-12-16T17:43:50Z

Thanks for the PR! I think we need to broaden the change to account for other (future) plugins, and not be specific to the tooltip UI.

If you look at src/components/markdown_editor/plugins/markdown_default_plugins.tsx there are two existing functions providing plugin arrays: getDefaultEuiMarkdownParsingPlugins and getDefaultEuiMarkdownProcessingPlugins. We should add a third one to return a default uiPlugins array, [MarkdownTooltip.plugin]. EuiMarkdownEditor would only need to be changed to not inject the tooltip plugin, instead passing uiPlugins directly to the toolbar component - maybe some extra work here depending on the following conversation:

@timroes this would be a breaking change, which is okay, but I think we could get by with a deprecation here by adding a flag to the return uiPlugins array which the editor could check for - if it's missing, it could continue injecting the tooltip button. Thoughts?

getDefaultEuiMarkdownUiPlugins = () => {
  const plugins = [MarkdownTooltip.plugin];
  plugins.__pregenerated = true;
  return plugins;
}

K-Kumar-01 · 2020-12-16T19:03:43Z

Thanks for the PR! I think we need to broaden the change to account for other (future) plugins, and not be specific to the tooltip UI.

If you look at src/components/markdown_editor/plugins/markdown_default_plugins.tsx there are two existing functions providing plugin arrays: getDefaultEuiMarkdownParsingPlugins and getDefaultEuiMarkdownProcessingPlugins. We should add a third one to return a default uiPlugins array, [MarkdownTooltip.plugin]. EuiMarkdownEditor would only need to be changed to not inject the tooltip plugin, instead passing uiPlugins directly to the toolbar component - maybe some extra work here depending on the following conversation:

@timroes this would be a breaking change, which is okay, but I think we could get by with a deprecation here by adding a flag to the return uiPlugins array which the editor could check for - if it's missing, it could continue injecting the tooltip button. Thoughts?
getDefaultEuiMarkdownUiPlugins = () => {
  const plugins = [MarkdownTooltip.plugin];
  plugins.__pregenerated = true;
  return plugins;
}

@chandlerprall
I will update my PR after the suggestion of @timroes, as you stated.

K-Kumar-01 · 2020-12-18T19:54:17Z

Thanks for the PR! I think we need to broaden the change to account for other (future) plugins, and not be specific to the tooltip UI.

If you look at src/components/markdown_editor/plugins/markdown_default_plugins.tsx there are two existing functions providing plugin arrays: getDefaultEuiMarkdownParsingPlugins and getDefaultEuiMarkdownProcessingPlugins. We should add a third one to return a default uiPlugins array, [MarkdownTooltip.plugin]. EuiMarkdownEditor would only need to be changed to not inject the tooltip plugin, instead passing uiPlugins directly to the toolbar component - maybe some extra work here depending on the following conversation:

@timroes this would be a breaking change, which is okay, but I think we could get by with a deprecation here by adding a flag to the return uiPlugins array which the editor could check for - if it's missing, it could continue injecting the tooltip button. Thoughts?
getDefaultEuiMarkdownUiPlugins = () => {
  const plugins = [MarkdownTooltip.plugin];
  plugins.__pregenerated = true;
  return plugins;
}

@timroes
Any updates here?

chandlerprall · 2020-12-21T16:20:17Z

A lot of us are now on holiday through the end of the year, and those remaining will start taking time this week. Since Tim's not here, let's go with my proposed setup to avoid a breaking change and see how that feels. 😁

K-Kumar-01 · 2020-12-22T18:26:46Z

@chandlerprall
I had a doubt here.

EuiMarkdownEditor would only need to be changed to not inject the tooltip plugin, instead passing uiPlugins directly to the toolbar component

Can you please explain what the above sentence means. I am unable to understand what we have to do here.

chandlerprall · 2020-12-22T20:14:29Z

Sure thing!

Right now, EuiMarkdownEditor receives an array of UI plugins https://github.com/elastic/eui/blob/master/src/components/markdown_editor/markdown_editor.tsx#L158

It then mixes that array with the internally-defined tooltip plugin https://github.com/elastic/eui/blob/master/src/components/markdown_editor/markdown_editor.tsx#L179

This prevents an application from removing the tooltip's toolbar UI, even if the tooltip processing has been disabled. The uiPlugins prop needs to be updated to work the same way parsingPluginList and processingPluginList are, which default to an internally-built list but can be fully overridden. One extra step is needed though, as doing this blindly for uiPlugins would not be backwards compatible:

uiPlugins will need to still be enhanced with MarkdownTooltip.plugin if the uiPlugins array did not originate from EUI's internal plugin array. This modifies the quote,

EuiMarkdownEditor would ~~only need to be changed to not inject the tooltip plugin,~~ need to inject the tooltip plugin if the array source is external to EUI, ~~instead~~ otherwise passing uiPlugins directly to the toolbar component

Let me know if that's still not clear!

K-Kumar-01 · 2020-12-23T18:20:42Z

Sure thing!

Right now, EuiMarkdownEditor receives an array of UI plugins https://github.com/elastic/eui/blob/master/src/components/markdown_editor/markdown_editor.tsx#L158

It then mixes that array with the internally-defined tooltip plugin https://github.com/elastic/eui/blob/master/src/components/markdown_editor/markdown_editor.tsx#L179

This prevents an application from removing the tooltip's toolbar UI, even if the tooltip processing has been disabled. The uiPlugins prop needs to be updated to work the same way parsingPluginList and processingPluginList are, which default to an internally-built list but can be fully overridden. One extra step is needed though, as doing this blindly for uiPlugins would not be backwards compatible:

uiPlugins will need to still be enhanced with MarkdownTooltip.plugin if the uiPlugins array did not originate from EUI's internal plugin array. This modifies the quote,

EuiMarkdownEditor would ~~only need to be changed to not inject the tooltip plugin,~~ need to inject the tooltip plugin if the array source is external to EUI, ~~instead~~ otherwise passing uiPlugins directly to the toolbar component

Let me know if that's still not clear!

@chandlerprall
Thanks for the explanation. I understood most of the concepts, though I still have a few doubts.

The parsingPluginList is passed as a pluggable list prop as far as I understand the code with default value as defaultParsingPluginList. Is the overriding you refer to here means passing it as props? Same for processingPluginList.
If overriding is the case above, then we should pass uiPlugins as a default value of internally built plugins if the user has not given any uiPlugins. In case there are uiPlugins passed, then also the tooltip plugin will be there unlike the above two, with the capability to be removed. Am I on the right track?

uiPlugins will need to still be enhanced with MarkdownTooltip.plugin if the uiPlugins array did not originate from EUI's internal plugin array.

Still not able to comprehend this part

chandlerprall · 2021-01-07T20:05:44Z

parsingPluginList and processingPluginList are correctly setup and represent what uiPlugins should have been from the beginning: a completely customizable list passed unmodified. The primary goal here is to make uiPlugins work like the other two do.

The part that's confusing here is to avoid making this a backwards-compatible change. For example, right now if I provide my own uiPlugins array the component also injects MarkdownTooltip.plugin. Switching uiPlugins to 100% the same pattern as the other two plugin props, that functionality is dropped. So I propose keeping track of if the uiPlugins array was created by the (to be created) getDefaultEuiMarkdownUiPlugins function, and if not then continue to inject the tooltip plugin.

Something like,

function getDefaultEuiMarkdownUiPlugins() {
  const array = [MarkdownTooltip.plugin];
  array.__originatedFromEui = true;
  return array;
}

... inside **EuiMarkdownEditor**

const toolbarPlugins = [...uiPlugins];
if (uiPlugins.__originatedFromEui !== true) toolbarPlugins.unshift(MarkdownTooltip.plugin);

This allows existing implementations to continue working, while allowing new implementations to call getDefaultEuiMarkdownUiPlugins and then remove the plugin.

timroes · 2021-01-11T09:33:22Z

@chandlerprall I unfortunately also had a couple of issues understanding the desired solution, so let me make sure that I understand it:

I am not really seeing how the proposed solution would prevent a breaking change? In the code you outlined above when a user passes in a custom array to uiPlugins, the code runs through the second part you outlined, it will detect that uiPlugins.__originatedFromEui is not true, and thus remove the MarkdownTooltip.plugin. But that means we're breaking behavior?

E.g. a user had the following code:

<EuiMarkdownEditor uiPlugins={[myFancyEmojiPlugin]} />

Before that they would get the fancy emoji plugin AND the the tooltip plugin, after that change they would only get the emoji plugin?

I am sorry for all the clarification, but could you potentially outline the consuming code, that you want to prevent breaking by that solution? (Btw I am not against a breaking change at all, just trying to understand the solution.)

chandlerprall · 2021-01-11T19:47:01Z

it will detect that uiPlugins.__originatedFromEui is not true, and thus remove the MarkdownTooltip.plugin

Other way around, it'll detect uiPlugins.__originatedFromEui is not true and then inject the plugin.

timroes · 2021-01-12T10:02:18Z

Meaning, if youa ctually want to get rid of it, you'll need to do something like the following?

const plugins = getDefaultEuiMarkdownUiPlugins();
plugins.splice(0, plugins.length);
plugins.push(yourPlugin);
<EuiMarkdownEditor uiPlugins={plugins} />

chandlerprall · 2021-01-12T16:24:46Z

Meaning, if youa ctually want to get rid of it, you'll need to do something like the following?

Correct. We'd mark the old functionality (injecting the tooltip) as deprecated and console.warn when it happens, but gives us room to make the change without being breaking.

K-Kumar-01 · 2021-01-14T11:15:40Z

@chandlerprall @timroes
I will try to implement the functionality as discussed in the comments and let you know if I face any difficulty.
Thanks for the clarification. :)

K-Kumar-01 · 2021-01-15T08:16:17Z

@chandlerprall
I implemented the above functionality and the code worked well. However at the time of commit I am getting this error.

I tried understanding the error and got to know that it is due to

Property '__originatedFromEui' does not exist on type '{ name: string; button: { label: string; iconType: string; }; formatting: { prefix: string; suffix: string; trimFirst: boolean; }; helpText: Element; }[]'

VS Code is showing some quick fixed though it changes the typescript lib file

Should I go for this fix or is there any other method possible?

chandlerprall · 2021-01-15T21:32:36Z

Let's tell TS to ignore the problem both where you set it on the array and then check for it later (which I assume will give the same error)

// @ts-ignore __originatedFromEui is a custom property
theArray.__originatedFromEui = true;

Added the ability to unregsiter inbuilt UI plugins Addresses issue 4239 Signed-off-by: k-kumar-01 <[email protected]>

Signed-off-by: k-kumar-01 <[email protected]>

K-Kumar-01 · 2021-01-16T16:55:32Z

Let's tell TS to ignore the problem both where you set it on the array and then check for it later (which I assume will give the same error)
// @ts-ignore __originatedFromEui is a custom property
theArray.__originatedFromEui = true;

@chandlerprall
I have made the changes to remove the tooltip plugin.
Let me know if there are any changes required.

chandlerprall · 2021-01-19T19:33:45Z

Tested, and it is working as expected. Thanks! Couple of items left:

The default value for uiPlugins should come from getDefaultEuiMarkdownUiPlugins, similar to defaults for parsingPluginList and processingPluginList
markdown_editor_with_plugins.js relies on this tooltip auto-inclusion and should be updated to use getDefaultEuiMarkdownUiPlugins
When adding the tooltip plugin let's include a deprecation message:

console.warn('Deprecation warning: uiPlugins passed to EuiMarkdownEditor does not include the tooltip plugin, which has been added for you. This automatic inclusion has been deprecated and will be removed in the future, see https://github.com/elastic/eui/pull/4383')

Default UI Plugins initialized Warning added on injecting tooltip plugin Signed-off-by: k-kumar-01 <[email protected]>

K-Kumar-01 · 2021-01-20T10:25:43Z

console.warn('Deprecation warning: uiPlugins passed to EuiMarkdownEditor does not include the tooltip plugin, which has been added for you. This automatic inclusion has been deprecated and will be removed in the future, see #4383')

@chandlerprall
If I am right, we should do this:

 if (!uiPlugins.__originatedFromEui) {
      toolbarPlugins.unshift(MarkdownTooltip.plugin);
      console.warn(
        'Deprecation warning: uiPlugins passed to EuiMarkdownEditor does not include the tooltip plugin, which has been added for you. This automatic inclusion has been deprecated and will be removed in the future, see https://github.com/elastic/eui/pull/4383'
      );
    }

I have added a PR making the above changes. Let me know if there are any changes required. :)

chandlerprall · 2021-01-21T16:50:14Z

Changes look good, I pushed an entry CHANGELOG.md & an update to the markdown plugin example page to use the new method. Will merge this after CI passes - jenkins test this

kibanamachine · 2021-01-21T17:12:18Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4383/

chandlerprall · 2021-01-21T19:36:40Z

looks like a flaky test run, jenkins test this

kibanamachine · 2021-01-21T19:59:10Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_4383/

chandlerprall

Sorry for the delay in merging @K-Kumar-01 , everything looks good; thanks for making these changes!

fix: Markdown editor unregister tooltip plugin

ae3a397

Issue number 4239 Ability to unregister the tooltip plugin added Updated the editor example doc to show how to use it

chandlerprall self-requested a review December 16, 2020 16:48

K-Kumar-01 added 3 commits January 16, 2021 22:15

fix:Markdown tooltip unregister

a223f2d

Added the ability to unregsiter inbuilt UI plugins Addresses issue 4239 Signed-off-by: k-kumar-01 <[email protected]>

Merge branches

8382882

Signed-off-by: k-kumar-01 <[email protected]>

Merge remote-tracking branch 'upstream/master'

f590cd3

K-Kumar-01 added 2 commits January 20, 2021 15:50

fix:Markdown ToolTip Plugin

fee847d

Default UI Plugins initialized Warning added on injecting tooltip plugin Signed-off-by: k-kumar-01 <[email protected]>

Merge remote-tracking branch 'upstream/master'

1be0f8d

chandlerprall added 2 commits January 21, 2021 09:46

Changelog & updated markdown plugins example

5892037

Merge branch 'master' into master

4c6bee9

chandlerprall approved these changes Jan 26, 2021

View reviewed changes

chandlerprall merged commit 34500b7 into elastic:master Jan 26, 2021

chandlerprall mentioned this pull request Jan 28, 2021

Upgrade EUI to v31.4.0 elastic/kibana#89648

Merged

chandlerprall mentioned this pull request Aug 18, 2021

[EuiMarkdownEditor] Add ability to unregister standard plugins (tooltip) #4239

Closed

i-a-n mentioned this pull request Sep 1, 2021

[EuiMarkdownEditor] Enhancement request: Smarter UI when no plugins present #5127

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Markdown editor unregister tooltip plugin #4383

fix: Markdown editor unregister tooltip plugin #4383

K-Kumar-01 commented Dec 15, 2020 •

edited by chandlerprall

Loading

cla-checker-service bot commented Dec 15, 2020 •

edited

Loading

kibanamachine commented Dec 15, 2020

elizabetdev commented Dec 15, 2020

elizabetdev commented Dec 15, 2020

kibanamachine commented Dec 15, 2020

K-Kumar-01 commented Dec 16, 2020

chandlerprall commented Dec 16, 2020 •

edited

Loading

K-Kumar-01 commented Dec 16, 2020

K-Kumar-01 commented Dec 18, 2020

chandlerprall commented Dec 21, 2020

K-Kumar-01 commented Dec 22, 2020

chandlerprall commented Dec 22, 2020

K-Kumar-01 commented Dec 23, 2020

chandlerprall commented Jan 7, 2021

timroes commented Jan 11, 2021 •

edited

Loading

chandlerprall commented Jan 11, 2021

timroes commented Jan 12, 2021 •

edited

Loading

chandlerprall commented Jan 12, 2021

K-Kumar-01 commented Jan 14, 2021

K-Kumar-01 commented Jan 15, 2021 •

edited

Loading

chandlerprall commented Jan 15, 2021

K-Kumar-01 commented Jan 16, 2021

chandlerprall commented Jan 19, 2021

K-Kumar-01 commented Jan 20, 2021

chandlerprall commented Jan 21, 2021

kibanamachine commented Jan 21, 2021

chandlerprall commented Jan 21, 2021

kibanamachine commented Jan 21, 2021

chandlerprall left a comment

fix: Markdown editor unregister tooltip plugin #4383

fix: Markdown editor unregister tooltip plugin #4383

Conversation

K-Kumar-01 commented Dec 15, 2020 • edited by chandlerprall Loading

Summary

Checklist

cla-checker-service bot commented Dec 15, 2020 • edited Loading

kibanamachine commented Dec 15, 2020

elizabetdev commented Dec 15, 2020

elizabetdev commented Dec 15, 2020

kibanamachine commented Dec 15, 2020

K-Kumar-01 commented Dec 16, 2020

chandlerprall commented Dec 16, 2020 • edited Loading

K-Kumar-01 commented Dec 16, 2020

K-Kumar-01 commented Dec 18, 2020

chandlerprall commented Dec 21, 2020

K-Kumar-01 commented Dec 22, 2020

chandlerprall commented Dec 22, 2020

K-Kumar-01 commented Dec 23, 2020

chandlerprall commented Jan 7, 2021

timroes commented Jan 11, 2021 • edited Loading

chandlerprall commented Jan 11, 2021

timroes commented Jan 12, 2021 • edited Loading

chandlerprall commented Jan 12, 2021

K-Kumar-01 commented Jan 14, 2021

K-Kumar-01 commented Jan 15, 2021 • edited Loading

chandlerprall commented Jan 15, 2021

K-Kumar-01 commented Jan 16, 2021

chandlerprall commented Jan 19, 2021

K-Kumar-01 commented Jan 20, 2021

chandlerprall commented Jan 21, 2021

kibanamachine commented Jan 21, 2021

chandlerprall commented Jan 21, 2021

kibanamachine commented Jan 21, 2021

chandlerprall left a comment

Choose a reason for hiding this comment

K-Kumar-01 commented Dec 15, 2020 •

edited by chandlerprall

Loading

cla-checker-service bot commented Dec 15, 2020 •

edited

Loading

chandlerprall commented Dec 16, 2020 •

edited

Loading

timroes commented Jan 11, 2021 •

edited

Loading

timroes commented Jan 12, 2021 •

edited

Loading

K-Kumar-01 commented Jan 15, 2021 •

edited

Loading