updating markdown options panel to EUI #18580

ppisljar · 2018-04-25T18:47:20Z

TODO:

tests

ppisljar · 2018-04-25T20:11:50Z

should we use panels to contain options ? (something like that is used in many visualizatons)

ppisljar · 2018-04-25T20:21:03Z

yeah, i think it looks better

elasticmachine · 2018-04-25T21:42:46Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-04-25T21:43:18Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cchaos · 2018-04-26T19:58:08Z

@ppisljar Are the three buttons above the settings a global component or are they manually added to each viz editor?

cchaos · 2018-04-26T21:12:07Z

@ppisljar Overall this is looking better. I found a few bugs and can offer a few suggestions for layout.

First, the bugs

1. Font size slider's initial state is wrong

When loading this screen the slider's handle is always on the far right no matter the actual font-size value.

Note: Eventually EUI will have a stepped slider to use for the font-sizes as increments.

2. Links in new tab switch doesn't take effect until after you refresh the page

3. When resizing the two panes, it will update the preview without having to press the update button

This seems like unexpected behavior to me as I feel like the only thing that should allow updating is the auto-update or update buttons.

4. Starting around 991px wide window size and below, we completely lose the preview

5. Adjusting the size of the textarea can mess up the pane sizing and cover up the preview

6. The help link is missing

However, the current help link is incorrect as it just goes to Github help and not markdown help. My suggestion is to change the textarea label text to "Content" and add the following link to the far right of the label.

<EuiLink href="https://www.markdownguide.org/cheat-sheet"><EuiIcon type="help" /> Syntax help</EuiLink>

Layout

7. If I could have my way 😁 I'd like to set up the layout like this:

Both the settings and preview are in a panel and the settings panel has the scrollbar. It also overrides some background colors and pushes the floating toggle buttons into the resizing area to maximize space and minimize overlapping.

cchaos

One more thing I saw there is a console error about supplying the rows property to the textarea as a string vs number.

ppisljar · 2018-05-03T12:53:04Z

thanks a lot for your feedback @cchaos

i will work on this today and try to move it forward, but in general i will try to keep the scope of this:

to only markdown visualization, so everything that is general for all visualizations will not be handled here
to only bugs introduced with this PR (so existing markdown bugs will not be fixed here, unless trivial)

ps: yes, the 3 buttons are the global editor. as well as panel resizing and floating toggle buttons.

maybe it would make sense to approach this from other direction and first EUIfy the editor (so everything around the actual option panels) ?

ppisljar · 2018-05-03T14:47:26Z

fixed:

help link
font slider is initialized
resizing works ok
styles updated a bit

not fixed:

when resizing changes get applied (problem on master)
open links in new tab has no effect until refreshing (problem on master)
on mobile we loose preview (problem on master)

no design changes were made to anything but the actual option panel of markdown editor

elasticmachine · 2018-05-03T17:05:46Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cchaos · 2018-05-03T22:34:29Z

Understandable about the scope issue for this PR. I'll leave it up to you in which order is best to progress with EUI-fying.

I really like your solution to the textarea sizing 🥇

The one other thing I'd really like to do is put the syntax link inline with the textarea label. This way it's always connected to the input (where they're actually writing the markup) in case more options are added in between.

I'd use <EuiFlexGroup> and <EuiFlexItem>s to wrap the label content and then just set the prop grow={false} on the flex item holding the link so that it sits at the far right.

elasticmachine · 2018-05-04T03:01:01Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

timroes · 2018-05-04T09:03:04Z

My suggestions regarding that PR:

Could we remove the "Markdown" header? We don't put a header with the vis type in any other editor, and imho we are just using space here. If we really want to print this somewhere (just for the markdown vis), I would rather have one editor tab named "Markdown" instead and being visible, but I really wouldn't like to have that huge title in the editor. (@cchaos your opinion on that?)
As already commented in the code, I think we should build that help text functionality into EUI, since we will have that same look and feel in many places in the editor. If we implement it everywhere like that, it makes it way harder to later change the styling of all those places in case we figure out a nicer way to have that hint texts. Also currently all of that will be in the "label" for the input field, which is kind of not the best accessibility, since it will always read out the textarea as "content syntax help", which might be confusing. Putting it into EUI, we could extract that help link in that (and all other places) from the label. (also pinging @snide on this one, because .. why not :D)

Besides those two suggestions, the general approach and look and feel looks good for me. Would rather have some decision on those two points before approving the PR :)

cchaos · 2018-05-04T16:21:53Z

@timroes

I'm fine with removing the large "Markdown" header. I agree that it is just taking up space. I'm not sure we needs to add tabs to just to add a heading and especially if there will only be one tab.

My one concern about removing it, though maybe we just move it to the textarea label ("Markdown content" or something), is that I was hoping that when we re-do the viz selector, we can change the "Markdown" option to just "Text". The reason being that, since we are now gearing Kibana towards non-engineer users, most people won't know what "Markdown" is, but if they see the option for "Text" they'll know what that is.

Kind of like in this mock:

As for you second bullet, I responded to your original about the helpText prop on EuiFormRow. We could just go ahead and use that though I'm concerned about pushing it to the bottom of a very long textarea.

timroes · 2018-05-04T16:40:03Z

I would be fine renaming the Markdown component to Text already, that would be an easy rename.

Regarding the adding one tab. Peter and I also discussed the possibility of splitting things up into two tabs, so we would have one tab "Text" or "Template" with just the input box available (and the help link) and a second tab "Options" to properly split the options, like we do in all other visualizations. In my opinion it was kind of an mistake we just plugged them all in that one tab. Do you think that would be a viable solution?

elasticmachine · 2018-05-08T17:41:50Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

timroes · 2018-05-09T12:39:55Z

Could you rebase on master to get rid of some of the test failures?

timroes · 2018-05-09T12:55:56Z

src/ui/public/vis/editors/components/editor_options_group.js

    <EuiPanel
      grow={false}
-      className="editorOptionsGroup__panel"
+      className={`editorOptionsGroup__panel ${props.className}`}


This would write editorOptionsGroup__panel undefined to the class if the property hasn't been specified. We usually use classnames for concatenating (and conditionally) apply classnames.

import className from 'classnames'; const panelClass = className('editorOptionsGroup__panel', props.className); return <EuiPanel className={panelClass} />;

timroes · 2018-05-09T12:56:17Z

src/ui/public/vis/editors/components/editor_options_group.js

@@ -18,10 +20,10 @@ import {
 * to produce an aligned look and feel.
 */
 function EditorOptionsGroup(props) {
-  return (
+  const collapsibleOptionGroup = (
    <EuiPanel
      grow={false}


I think we want to use prop.grow here too :-)

grow on panel with accordion doesn't make any sense as accordion does not grow

In that case I would add the following to the propType definition of grow: "Only applies when collapsible is set to false."

timroes · 2018-05-09T12:57:49Z

src/ui/public/vis/editors/components/editor_options_group.js

+            </EuiFlexItem>
+          </EuiFlexGroup>
+
+          <EuiSpacer size="m"/>


Panel layouts it's content different than accordion. Looking in the rendered result, I would suggest that we remove the EuiSpacer in this case, since it would introduce noticeable more spacing than the EuiAccordion variant would have.

timroes · 2018-05-09T13:03:17Z

src/core_plugins/markdown_vis/public/markdown_vis_editor.js

+    const params = this.props.scope.vis.params;
+
+    const markdownLabel = (
+      <EuiLink className="markdown-vis--help" href="https://www.markdownguide.org/cheat-sheet" target="_blank">


I think the class name can be removed. It doesn't seem to be used anywhere.

timroes · 2018-05-09T13:07:30Z

src/core_plugins/markdown_vis/public/markdown_vis.less


+.markdown-vis-editor textarea {


We could instead of needing the nested query, just using className to apply a class name to the EuiTextArea instead.

elasticmachine · 2018-05-09T14:11:05Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-05-09T17:54:28Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

timroes

Add some tests and that LGTM

nreese · 2018-05-10T13:56:45Z

You can put the UI in a weird state by editing the markdown, then clicking Apply changes, and finally clicking the settings tab. Notice how the settings are not contained in the general panel are instead displayed on the grey background.

timroes · 2018-05-10T14:17:45Z

So this is also blocked by elastic/eui#805

elasticmachine · 2018-05-10T15:59:26Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

ppisljar · 2018-06-18T07:19:41Z

closing, as the bug in eui will not be fixed and we will not be euifying our editor for now

ppisljar added review Team:Platform-Design Team Label for Kibana Design Team. Support the Analyze group of plugins. Feature:Visualizations Generic visualization features (in case no more specific feature label is available) v6.4.0 labels Apr 25, 2018

ppisljar requested review from cchaos, timroes and thomasneirynck April 25, 2018 18:47

cchaos suggested changes Apr 26, 2018

View reviewed changes

ppisljar force-pushed the ang/markdownOptionsEUI branch from 541d573 to cd16f21 Compare May 8, 2018 15:34

timroes reviewed May 9, 2018

View reviewed changes

updating markdown options panel to EUI

af03117

ppisljar added 7 commits May 9, 2018 08:04

adding EuiPanel

4e11ff7

updating based on review

6d1196e

fixing console error

20ba66b

moving help next to content label

ad1cd61

removing unnecesary styles

f62b5b7

spliting into two tabs and rebasing

942919e

updating

fc47132

timroes reviewed May 9, 2018

View reviewed changes

updating based on review

afad250

ppisljar force-pushed the ang/markdownOptionsEUI branch from 788bbea to afad250 Compare May 9, 2018 13:12

fixing test

52c7145

timroes approved these changes May 10, 2018

View reviewed changes

adding tests

9bc83a7

ppisljar requested a review from nreese May 10, 2018 13:38

timroes added the blocked label May 10, 2018

ppisljar closed this Jun 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

updating markdown options panel to EUI #18580

updating markdown options panel to EUI #18580

ppisljar commented Apr 25, 2018 •

edited

Loading

ppisljar commented Apr 25, 2018 •

edited

Loading

ppisljar commented Apr 25, 2018

elasticmachine commented Apr 25, 2018

elasticmachine commented Apr 25, 2018

cchaos commented Apr 26, 2018

cchaos commented Apr 26, 2018

cchaos left a comment

ppisljar commented May 3, 2018

ppisljar commented May 3, 2018

elasticmachine commented May 3, 2018

cchaos commented May 3, 2018

elasticmachine commented May 4, 2018

timroes commented May 4, 2018

cchaos commented May 4, 2018

timroes commented May 4, 2018

elasticmachine commented May 8, 2018

timroes commented May 9, 2018

timroes May 9, 2018

timroes May 9, 2018

ppisljar May 9, 2018

timroes May 9, 2018

timroes May 9, 2018

timroes May 9, 2018

timroes May 9, 2018

elasticmachine commented May 9, 2018

elasticmachine commented May 9, 2018

timroes left a comment

nreese commented May 10, 2018 •

edited

Loading

timroes commented May 10, 2018

elasticmachine commented May 10, 2018

ppisljar commented Jun 18, 2018


		.markdown-vis-editor textarea {

updating markdown options panel to EUI #18580

updating markdown options panel to EUI #18580

Conversation

ppisljar commented Apr 25, 2018 • edited Loading

ppisljar commented Apr 25, 2018 • edited Loading

ppisljar commented Apr 25, 2018

elasticmachine commented Apr 25, 2018

💔 Build Failed

elasticmachine commented Apr 25, 2018

💔 Build Failed

cchaos commented Apr 26, 2018

cchaos commented Apr 26, 2018

First, the bugs

1. Font size slider's initial state is wrong

2. Links in new tab switch doesn't take effect until after you refresh the page

3. When resizing the two panes, it will update the preview without having to press the update button

4. Starting around 991px wide window size and below, we completely lose the preview

5. Adjusting the size of the textarea can mess up the pane sizing and cover up the preview

6. The help link is missing

Layout

7. If I could have my way 😁 I'd like to set up the layout like this:

cchaos left a comment

Choose a reason for hiding this comment

ppisljar commented May 3, 2018

ppisljar commented May 3, 2018

elasticmachine commented May 3, 2018

💔 Build Failed

cchaos commented May 3, 2018

elasticmachine commented May 4, 2018

💔 Build Failed

timroes commented May 4, 2018

cchaos commented May 4, 2018

timroes commented May 4, 2018

elasticmachine commented May 8, 2018

💔 Build Failed

timroes commented May 9, 2018

timroes May 9, 2018

Choose a reason for hiding this comment

timroes May 9, 2018

Choose a reason for hiding this comment

ppisljar May 9, 2018

Choose a reason for hiding this comment

timroes May 9, 2018

Choose a reason for hiding this comment

timroes May 9, 2018

Choose a reason for hiding this comment

timroes May 9, 2018

Choose a reason for hiding this comment

timroes May 9, 2018

Choose a reason for hiding this comment

elasticmachine commented May 9, 2018

💔 Build Failed

elasticmachine commented May 9, 2018

💚 Build Succeeded

timroes left a comment

Choose a reason for hiding this comment

nreese commented May 10, 2018 • edited Loading

timroes commented May 10, 2018

elasticmachine commented May 10, 2018

💔 Build Failed

ppisljar commented Jun 18, 2018

ppisljar commented Apr 25, 2018 •

edited

Loading

ppisljar commented Apr 25, 2018 •

edited

Loading

nreese commented May 10, 2018 •

edited

Loading