[Canvas] Adds argument to open all links in new tab within markdown element #57017

maggieghamry · 2020-02-06T18:20:09Z

Summary

Fixes Option to set Markdown elments URL to open in new tab #49314
Added option to open links in new tab from markdown element
Updated view to add true/false toggle to markdown element sidebar
Added i18n strings for the new argument
Added help definition and label text
Images:

Note: will update function reference docs in a separate PR

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-02-06T18:53:29Z

Pinging @elastic/kibana-canvas (Team:Canvas)

timductive · 2020-02-06T19:03:59Z

This looks awesome and thorough! Just a note can you add "fixes #issuenumber" to the PR comment and tag the issue so it auto-closes when this merges?

crob611 · 2020-02-06T19:11:06Z

x-pack/legacy/plugins/canvas/canvas_plugin_src/renderers/markdown/index.js

  render(domNode, config, handlers) {
-    const html = { __html: md.render(String(config.content)) };
    const fontStyle = config.font ? config.font.spec : {};
+    const openLinksInNewTab = config.openLinksInNewTab;


Rather than copy all of this logic in here, we should import {Markdown} from 'src/legacy/core_plugins/kibana_react/public/markdown'

Then this method can just become something like

ReactDOM.render( <Markdown className="canvasMarkdown" markdown={config.content} openLinksInNewtabs={config.openLinksInNewTab} />

@crob611 I tried this import, but had to add to the method with an else statement, as using it as-is doesn't account for the default undefined state.

Using the original method, if you toggle to true, the links will open in a new tab, but if you then toggle to false (with the original method), it persists the true value (links still opened in a new tab).

With the modification; adding the the else statement, it accounts for the default (undefined), explicit true, and explicit false conditions.

I'm not sure how best to append the condition to the imported method.

@crob611 do you have any suggestions on how best to append the else condition to the imported method? I'm not sure what the impact of adding the condition to the original method would be.

@crob611 I've updated the code per your help/recommendation, and got the text not to wrap, but looks like the toggle example from the bar chart is using axisConfig (when I specified argType of axisConfig instead of toggle, it looked just like the Y-axis section, but the toggleArgInput seems to need to be updated with different styling to render it the same way. I'm working to understand this a bit better.

ryankeairns · 2020-02-06T19:26:03Z

Thanks for tackling this!

I had envisioned the switch being inside the 'Markdown content' section as opposed to its own section, but is the reason for placing it into its own section due to the fact that there can be many 'Markdown content' sections in the side panel? meaning the single switch then applies to all sections?

As for the design, let's make the text fit on one line by increasing the label width and align the switch to the right side (see the Y-axis example below).

Additionally, we can shorten the text to 'Open links in new tab'.

elasticmachine · 2020-02-06T19:39:56Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: d6ee571

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

maggieghamry · 2020-02-06T21:14:30Z

Thanks for tackling this!

I had envisioned the switch being inside the 'Markdown content' section as opposed to its own section, but is the reason for placing it into its own section due to the fact that there can be many 'Markdown content' sections in the side panel? meaning the single switch then applies to all sections?

As for the design, let's make the text fit on one line by increasing the label width and align the switch to the right side (see the Y-axis example below).

Additionally, we can shorten the text to 'Open links in new tab'.

@ryankeairns yes, based on the Kibana example, the toggle is in its own separate section. Since the markdown content is specific to the code inside of the element, and adding it separately allows for the option to be added to the other options in the view side bar. (see the first screenshot):

I'll work on updating the layout for the text as well. @ryankeairns can you please let me know which element you sent the screenshot example from?

crob611 · 2020-02-07T17:35:24Z

@ryankeairns Our default "toggle" argument type that is used in other places is what's causing the text to fill 1/3 of the space and the toggle element to be pushed left. Here it is used elsewhere. Guess it's that way so all the controls align with each other?

Should we alter that toggle type so that everywhere the toggle switch aligns right and the rest of the text can fill in as much space as needed?

Update to change toggle verbiage per Ryan's request, and reuse the Kibana Markdown per Corey's help and recommendation. Still working on updating the styles (consulting Ryan)

Update to prevent text for "Open links in new tab from wrapping" - the example from the horizontal bar chart is quite differently, and reads from "axisConfig" - when I changed the argType to "axisConfig", the layout was the same, but I'll need some input on which specific styles to add to the "ToggleArgInput" - I think this is where the style updates need to occur to get the toggle to stay on the same line, but may be wrong.

Update to original message string per Ryan's feedback, now that there is no wrapping

ryankeairns · 2020-02-07T22:00:56Z

@crob611 yep. We may want to consider overriding the underlying layout styles and make the label column wider. Maggie and I are working about it for this PR, but I'll keep an eye on it for future consideration.

maggieghamry · 2020-02-07T23:03:07Z

@ryankeairns @crob611 I've updated the toggle per Ryan's request (also updated screenshots)

ryankeairns

Looking great, one small change request for the text in the popover.

I also noticed that the (recently changed in EUI) focus state on the switch gets clipped a bit, but that will require some more thought on how to address since any additional styles will affect all sidebar panels. Anyway, its minor and we can address it later.

x-pack/legacy/plugins/canvas/i18n/ui.ts

…open-md-links-new-tab

ryankeairns

Thanks for the changes, LGTM

crob611 · 2020-02-18T14:49:47Z

@maggieghamry Make sure to merge in my pull request to your branch to use the <Markdown component, and then I think this will be ready to be merged. Let me know if you have any questions about how to do that.

maggieghamry · 2020-02-18T16:49:45Z

@maggieghamry Make sure to merge in my pull request to your branch to use the <Markdown component, and then I think this will be ready to be merged. Let me know if you have any questions about how to do that.

@crob611 sorry, I actually added the code in that you recommended, rather than merging the pull request. Do I still need to merge that in, even though I updated the code?
Update: @crob611 I went ahead and merged the PR

Switch to using Markdown Component

update to fix internationalization issues

crob611

Looks great. Nice work!

…lement (elastic#57017) * Adds toggle to open links in new tab within markdown element * Updating test for markdown function * Switch to using Markdown Component * Update ui.ts Update to change toggle verbiage per Ryan's request, and reuse the Kibana Markdown per Corey's help and recommendation. Still working on updating the styles (consulting Ryan) * Update toggle.js Update to prevent text for "Open links in new tab from wrapping" - the example from the horizontal bar chart is quite differently, and reads from "axisConfig" - when I changed the argType to "axisConfig", the layout was the same, but I'll need some input on which specific styles to add to the "ToggleArgInput" - I think this is where the style updates need to occur to get the toggle to stay on the same line, but may be wrong. * Update ui.ts Update to original message string per Ryan's feedback, now that there is no wrapping * Update to UI styles per Ryan's feedback * updating message per Ryan's request * Update ui.ts update to fix internationalization issues Co-authored-by: Corey Robertson <crob611@gmail.com>

kibanamachine · 2020-02-20T17:25:53Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: c344399

History

💚 Build #27727 succeeded c344399
💔 Build #27430 failed f6e216d
💔 Build #27200 failed 19119ca
💔 Build #25392 failed 5211fde
💔 Build #25368 failed 1f70cf3

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…lement (#57017) (#58149) * Adds toggle to open links in new tab within markdown element * Updating test for markdown function * Switch to using Markdown Component * Update ui.ts Update to change toggle verbiage per Ryan's request, and reuse the Kibana Markdown per Corey's help and recommendation. Still working on updating the styles (consulting Ryan) * Update toggle.js Update to prevent text for "Open links in new tab from wrapping" - the example from the horizontal bar chart is quite differently, and reads from "axisConfig" - when I changed the argType to "axisConfig", the layout was the same, but I'll need some input on which specific styles to add to the "ToggleArgInput" - I think this is where the style updates need to occur to get the toggle to stay on the same line, but may be wrong. * Update ui.ts Update to original message string per Ryan's feedback, now that there is no wrapping * Update to UI styles per Ryan's feedback * updating message per Ryan's request * Update ui.ts update to fix internationalization issues Co-authored-by: Corey Robertson <crob611@gmail.com> Co-authored-by: Corey Robertson <crob611@gmail.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

maggieghamry added 2 commits February 6, 2020 12:59

Adds toggle to open links in new tab within markdown element

43a0a6b

Updating test for markdown function

d6ee571

maggieghamry added the Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// label Feb 6, 2020

maggieghamry added enhancement New value added to drive a business result release_note:enhancement impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:medium Medium Level of Effort v8.0.0 v7.7.0 labels Feb 6, 2020

maggieghamry marked this pull request as ready for review February 6, 2020 18:56

maggieghamry requested a review from a team as a code owner February 6, 2020 18:56

crob611 reviewed Feb 6, 2020

View reviewed changes

ryankeairns self-requested a review February 6, 2020 19:15

Switch to using Markdown Component

1483cdc

maggieghamry added 3 commits February 7, 2020 13:37

Update ui.ts

04602b1

Update to change toggle verbiage per Ryan's request, and reuse the Kibana Markdown per Corey's help and recommendation. Still working on updating the styles (consulting Ryan)

Update ui.ts

535d0a7

Update to original message string per Ryan's feedback, now that there is no wrapping

Update to UI styles per Ryan's feedback

5211fde

ryankeairns requested changes Feb 10, 2020

View reviewed changes

x-pack/legacy/plugins/canvas/i18n/ui.ts Outdated Show resolved Hide resolved

maggieghamry added 2 commits February 17, 2020 10:51

Merge branch 'master' of https://github.com/elastic/kibana into feat/…

184e960

…open-md-links-new-tab

updating message per Ryan's request

19119ca

crob611 added the review label Feb 18, 2020

ryankeairns self-requested a review February 18, 2020 14:43

ryankeairns approved these changes Feb 18, 2020

View reviewed changes

Merge pull request #1 from crob611/pr/57017

f6e216d

Switch to using Markdown Component

maggieghamry requested a review from crob611 February 18, 2020 18:43

Update ui.ts

c344399

update to fix internationalization issues

crob611 approved these changes Feb 20, 2020

View reviewed changes

maggieghamry merged commit 3cba051 into elastic:master Feb 20, 2020

timductive mentioned this pull request Feb 20, 2020

Option to set Markdown elments URL to open in new tab #49314

Closed

maggieghamry mentioned this pull request Feb 20, 2020

[7.x] [Canvas] Adds argument to open all links in new tab within markdown element (#57017) #58149

Merged

clintandrewhall mentioned this pull request Jun 4, 2020

[Canvas] Fixes for expected prop-types, Shape Arg UI, and i18n strings #68304

Merged

2 tasks

[Canvas] Adds argument to open all links in new tab within markdown element #57017

[Canvas] Adds argument to open all links in new tab within markdown element #57017

Uh oh!

Conversation

maggieghamry commented Feb 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Feb 6, 2020

Uh oh!

timductive commented Feb 6, 2020

Uh oh!

crob611 Feb 6, 2020

Choose a reason for hiding this comment

Uh oh!

maggieghamry Feb 6, 2020 • 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!

maggieghamry Feb 7, 2020 • 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!

ryankeairns commented Feb 6, 2020

Uh oh!

elasticmachine commented Feb 6, 2020

💚 Build Succeeded

Uh oh!

maggieghamry commented Feb 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crob611 commented Feb 7, 2020

Uh oh!

ryankeairns commented Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maggieghamry commented Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryankeairns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryankeairns left a comment

Choose a reason for hiding this comment

Uh oh!

crob611 commented Feb 18, 2020

Uh oh!

maggieghamry commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crob611 left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Feb 20, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

maggieghamry commented Feb 6, 2020 •

edited

Loading

maggieghamry Feb 6, 2020 •

edited

Loading

maggieghamry Feb 7, 2020 •

edited

Loading

maggieghamry commented Feb 6, 2020 •

edited

Loading

ryankeairns commented Feb 7, 2020 •

edited

Loading

maggieghamry commented Feb 7, 2020 •

edited

Loading

maggieghamry commented Feb 18, 2020 •

edited

Loading