Add vertical scroll and text highlighting for functions #3918

ashikmeerankutty · 2020-08-14T16:36:06Z

Summary

Makes progress on #3056

Add a vertical scroll for types with lengths exceeding type container .
Add highlighting for functions

Before:

After:

Checklist

Check against all themes for compatibility in both light and dark modes
Checked in mobile
Checked in Chrome, Safari,
Checked in Edge, and Firefox
- [ ] Props have proper autodocs
- [ ] Added 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~~

kibanamachine · 2020-08-14T16:36:08Z

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?

ashikmeerankutty · 2020-08-14T16:45:07Z

This is the result if we add text highlight for all types

Did it look good ?

cchaos · 2020-08-14T18:19:06Z

I think the code highlight is a great advantage!

I will say though, I really think it's important not to force text-wrap on code elements. For the same reason why code editors don't wrap lines by default. It's really hard to read what is the whole word.

That said, I haven't thought through a real solution, I just know that forcing word-wrap should not be the solution here.

ashikmeerankutty · 2020-08-14T18:22:06Z

That said, I haven't thought through a real solution, I just know that forcing word-wrap should not be the solution here.

@chandlerprall suggested adding a vertical scroll for functions so they will be displayed in a single line. I tried that but I couldn't find which component is causing the word-wrap

cchaos · 2020-08-14T18:34:30Z

Jenkins, test this

kibanamachine · 2020-08-14T18:59:02Z

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

src-docs/src/components/guide_section/guide_section.js

thompsongl · 2020-08-18T16:33:52Z

jenkins test this

kibanamachine · 2020-08-18T16:58:51Z

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

thompsongl · 2020-08-18T21:53:25Z

I tried that but I couldn't find which component is causing the word-wrap

.euiTableCellContent__text is responsible for the word-wrap/word-break alterations.
Looks like if you add className="eui-textBreakNormal" to the EuiCode element, more "natural" word wrapping occurs.

ashikmeerankutty · 2020-08-19T13:17:45Z

eui-textBreakNormal

So Is there anyway to remove the word wrap completely to use vertical scrolling. I think that would be more readable for functions.

thompsongl · 2020-08-19T15:17:20Z

Is there anyway to remove the word wrap completely

Likely with CSS, yes.
I think the current changes (with the addition of eui-textBreakNormal) are good for now, but we should have a designer take a look at adjusting the table layout. For instance, the table in the Playground tab moved the 'Note' column, which allows for more available space. Thoughts @cchaos?

cchaos · 2020-08-19T15:45:54Z

Sorry, I think I'm a bit confused as to what this is trying to accomplish. I don't think we should limit the height of any of the cells enforcing a scrollbar. They should always be fully visible. If that means we have to have word breaks, that's fine for now.

thompsongl · 2020-08-19T16:02:27Z

Ah, yeah, so this doesn't add vertical scroll, it add horizontal scroll if necessary. Height is never restricted.

ashikmeerankutty · 2020-08-19T16:40:06Z

So Is there anyway to remove the word wrap completely to use vertical scrolling. I think that would be more readable for functions.

I meant horizontal scroll everywhere in place of vertical scroll. Sorry I messed !!

cchaos · 2020-08-19T18:14:42Z

src-docs/src/components/guide_section/guide_section.js

+          if (functionMatches[j]) {
+            elements.push(
+              <EuiCode>
+                <span className="eui-textBreakNormal">{types[i]}</span>
+              </EuiCode>
+            );
+            elements.push(
+              <EuiCode language="ts">{functionMatches[j][0]}</EuiCode>
+            );
+            j++;
+          } else {
+            elements.push(
+              <EuiCode>
+                <span className="eui-textBreakNormal">{types[i]}</span>
+              </EuiCode>
+            );
+          }


I'm working on a direct fix for the overflow stuff, but can you help me better understand when each of these get displayed? Like what does functionMatches do?

Yes, my idea actually here is to find all functions and display them inline using horizontal scroll as suggested here #3554 (review) under long type rendering. I wasn't able to achieve that due to word-wraps. so that I commented it here but unfortunately. I used vertical instead of horizontal and messed things.

Attached screenshot as comment link seems not working as it was a review

I'm working on a direct fix for the overflow stuff, but can you help me better understand when each of these get displayed? Like what does functionMatches do?

Also functions may appear sometimes in enums also so in that case i tried to extract functions from those enums using regex also

Ah ok, so we want the multiple version to horizontally scroll. Ok, let me actually keep playing with my branch off of yours.

Fine, that was the idea.

cchaos · 2020-08-19T18:41:38Z

@ashikmeerankutty I had to play around with how the table cell styles were interacting with the code blocks so I just ended up creating a PR for you. ashikmeerankutty#6

I see though that all Fragments still need a key prop

ashikmeerankutty · 2020-08-19T18:44:42Z

I see though that all code code blocks still need a key prop

Thank you so much. I will add them

Swap usages of code for code block and fix overflow

thompsongl · 2020-08-19T20:10:05Z

jenkins test this

thompsongl · 2020-08-19T20:30:32Z

jenkins test this

kibanamachine · 2020-08-19T20:59:15Z

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

thompsongl

Looks great!

* Add vertical scroll and text highlighting for functions * use code block instead of markdown * Swap usages of code for code block and fix overflow * One codeblock with line breaks when multiple functions * Fixed warnings of missing keys Co-authored-by: cchaos <caroline.horn@elastic.co>

Add vertical scroll and text highlighting for functions

872509a

cchaos reviewed Aug 14, 2020

View reviewed changes

src-docs/src/components/guide_section/guide_section.js Outdated Show resolved Hide resolved

use code block instead of markdown

f47b025

cchaos reviewed Aug 19, 2020

View reviewed changes

Swap usages of code for code block and fix overflow

a657af9

cchaos and others added 2 commits August 19, 2020 14:54

One codeblock with line breaks when multiple functions

eb69f23

Merge pull request #6 from cchaos/ashikmeerankutty-vertical-scroll

2acc2e0

Swap usages of code for code block and fix overflow

Fixed warnings of missing keys

410afc4

thompsongl approved these changes Aug 19, 2020

View reviewed changes

thompsongl merged commit d2ab274 into elastic:master Aug 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add vertical scroll and text highlighting for functions #3918

Add vertical scroll and text highlighting for functions #3918

ashikmeerankutty commented Aug 14, 2020

kibanamachine commented Aug 14, 2020

ashikmeerankutty commented Aug 14, 2020

cchaos commented Aug 14, 2020

ashikmeerankutty commented Aug 14, 2020

cchaos commented Aug 14, 2020

kibanamachine commented Aug 14, 2020

thompsongl commented Aug 18, 2020

kibanamachine commented Aug 18, 2020

thompsongl commented Aug 18, 2020

ashikmeerankutty commented Aug 19, 2020

thompsongl commented Aug 19, 2020

cchaos commented Aug 19, 2020

thompsongl commented Aug 19, 2020

ashikmeerankutty commented Aug 19, 2020

cchaos Aug 19, 2020

ashikmeerankutty Aug 19, 2020

ashikmeerankutty Aug 19, 2020

ashikmeerankutty Aug 19, 2020

cchaos Aug 19, 2020

ashikmeerankutty Aug 19, 2020

cchaos commented Aug 19, 2020 •

edited

Loading

ashikmeerankutty commented Aug 19, 2020

thompsongl commented Aug 19, 2020

thompsongl commented Aug 19, 2020

kibanamachine commented Aug 19, 2020

thompsongl left a comment

Add vertical scroll and text highlighting for functions #3918

Add vertical scroll and text highlighting for functions #3918

Conversation

ashikmeerankutty commented Aug 14, 2020

Summary

Before:

After:

Checklist

kibanamachine commented Aug 14, 2020

ashikmeerankutty commented Aug 14, 2020

cchaos commented Aug 14, 2020

ashikmeerankutty commented Aug 14, 2020

cchaos commented Aug 14, 2020

kibanamachine commented Aug 14, 2020

thompsongl commented Aug 18, 2020

kibanamachine commented Aug 18, 2020

thompsongl commented Aug 18, 2020

ashikmeerankutty commented Aug 19, 2020

thompsongl commented Aug 19, 2020

cchaos commented Aug 19, 2020

thompsongl commented Aug 19, 2020

ashikmeerankutty commented Aug 19, 2020

cchaos Aug 19, 2020

Choose a reason for hiding this comment

ashikmeerankutty Aug 19, 2020

Choose a reason for hiding this comment

ashikmeerankutty Aug 19, 2020

Choose a reason for hiding this comment

ashikmeerankutty Aug 19, 2020

Choose a reason for hiding this comment

cchaos Aug 19, 2020

Choose a reason for hiding this comment

ashikmeerankutty Aug 19, 2020

Choose a reason for hiding this comment

cchaos commented Aug 19, 2020 • edited Loading

ashikmeerankutty commented Aug 19, 2020

thompsongl commented Aug 19, 2020

thompsongl commented Aug 19, 2020

kibanamachine commented Aug 19, 2020

thompsongl left a comment

Choose a reason for hiding this comment

cchaos commented Aug 19, 2020 •

edited

Loading