Add display, descriptionWidth, textWrap and isInvalid props to EuiExpression by andreadelrio · Pull Request #3467 · elastic/eui

andreadelrio · 2020-05-12T19:56:15Z

Summary

This PR adds the display, descriptionWidth, textWrap and isInvalid props to EuiExpression. The improvements introduced in this PR come, in part, as response to this Kibana ticket.

There are three new sections in the docs. One where display and descriptionWidth are shown and two others for textWrap and isInvalid respectively.

New: Column display

New: Invalid state for both clickable and read-only Expressions

New: Text truncation

Checklist

Check against all themes for compatibility in both light and dark modes
Checked in mobile
Checked in IE11 and Firefox
Props have proper autodocs
Added documentation 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-05-12T20:18:07Z

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

kibanamachine · 2020-05-13T03:24:11Z

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

kibanamachine · 2020-05-13T05:01:28Z

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

kibanamachine · 2020-05-13T06:03:36Z

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

mdefazio · 2020-05-13T12:06:15Z

This looks good. The demo example wasn't showing the truncate prop however. And do we need a min-width? Perhaps just enough so 2 letter operators still show?

cchaos

This thing is finally getting used and getting some ❤️ love.

src/components/expression/expression.tsx

src/components/expression/_expression.scss

src-docs/src/views/expression/truncate.tsx

kibanamachine · 2020-05-13T23:17:04Z

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

andreadelrio · 2020-05-13T23:38:57Z

This looks good. The demo example wasn't showing the truncate prop however. And do we need a min-width? Perhaps just enough so 2 letter operators still show?

@mdefazio not sure what you mean by this, can you elaborate? do you mean a min-width for the description (left side) or for EuiExpression as a whole?

andreadelrio · 2020-05-13T23:44:33Z

@cchaos wondering if it's wrong having to set a width on EuiExpression in order to have truncation work?

mdefazio · 2020-05-14T12:48:28Z

do you mean a min-width for the description (left side) or for EuiExpression as a whole?

I meant originally for the description (left-side), however maybe it should be as a whole—not sure which is right though tbh.

Some thoughts as I play around with this some more:

Should we allow the expressions to wrap as the container shrinks (assuming the expression itself can still fit on one line)?
If the expression can't fit on one line, then we truncate it.
Does it become tricky then if a 2nd expression is added dynamically based on the choice of the first? Do we want this to stay on the same line? Or simply allow it to drop to a new one? I know in my original mock I showed it on the same line, but I understand this presents some issues. Perhaps in this scenario, both expressions are treated as one whole so we can truncate the only the end of it without the 2nd expression breaking to a new line. Not sure if this is possible though, or doesn't just create more trouble than good.
If these are true, then I'm guessing we would need the tooltip on every element, since we wouldn't know when it gets truncated?

kibanamachine · 2020-05-29T20:39:53Z

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

Review updates

kibanamachine · 2020-05-29T23:19:07Z

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

andreadelrio · 2020-06-01T04:55:40Z

There also still seems to be a bit of a truncation issue. It might still be nice for consumers to be able to adjust this with a simple prop. Similar to how EuiCodeEditor handles whiteSpace you do for `wordWrap: 'truncate' | 'breakWord'.

@cchaos do you think that truncation should be for both the description and the value? or just the value? I would think that just for the value.

And another doubt, what do you think the truncate behaviour should be on the second Expression here ^? Truncate so that it only takes up one line? Like this

kibanamachine · 2020-06-01T05:56:13Z

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

cchaos · 2020-06-01T15:38:35Z

I would assume you'd truncate both if they got so wide that they extended the full width of the container. If this isn't exactly how the consumer wants the behavior, they can add custom wrappers to their own implementation.

But I think it's more obvious what the component will do unless you specify a different wrap/truncation prop for each part (but we shouldn't need to go down that route).

andreadelrio · 2020-06-01T18:17:11Z

@cchaos I added the textWrap prop and added a new example for it.

andreadelrio · 2020-06-01T18:18:38Z

@cchaos @mdefazio this should be ready for review. Can't re-request so pinging you this way. Thanks

cchaos

This is looking really good. It maintains the original ideals of the component while simply making it more robust with more alternate displays. It might be a good idea to update the PR summary screenshots.

One addition to the truncate prop we may want to consider is using EuiInnerText to apply the rendered text as the title prop to both the description and values. This will ensure that the full truncated text can be seen with a long mouse hover.

Most of my other comments are copy changes and best practices. But truly this is in a great spot!

CHANGELOG.md

src-docs/src/views/expression/columns.js

src-docs/src/views/expression/expression_example.js

src/components/expression/expression.tsx

src/components/expression/_expression.scss

kibanamachine · 2020-06-02T00:43:43Z

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

mdefazio

This looks great! Nice work!

cchaos

🎉 LGTM! Just a few nit comments now. I also added Greg for an eng for review.

cchaos · 2020-06-02T14:46:25Z

src-docs/src/views/expression/columns.js

+  const [example1, setExample1] = useState({
+    isOpen: false,
+    value: (
+      <div>


I don't think you need this div, but if you do, it gets removed if you change the selected options, so you need to add it to the onChange function as well. You can simply change it to a fragment <></>

cchaos · 2020-06-02T14:55:30Z

src-docs/src/views/expression/columns.js

+    setExample2({
+      ...example2,
+    });


Suggested change

setExample2({

...example2,

});

cchaos · 2020-06-02T14:55:46Z

src-docs/src/views/expression/columns.js

+    setExample1({
+      ...example1,
+    });


Suggested change

setExample1({

...example1,

});

cchaos · 2020-06-02T14:59:21Z

src-docs/src/views/expression/expression_example.js

+      text: (
+        <p>
+          To truncate <strong>EuiExpression</strong>&apos;s content, pass{' '}
+          <EuiCode>truncate</EuiCode> to the <EuiCode>textWrap</EuiCode> prop.


Suggested change

<EuiCode>truncate</EuiCode> to the <EuiCode>textWrap</EuiCode> prop.

<EuiCode language="ts">{'textWrap="truncate"'}</EuiCode>.

cchaos · 2020-06-02T14:59:43Z

src-docs/src/views/expression/expression_example.js

+          Text truncation only works properly if the prop types of{' '}
+          <EuiCode>description</EuiCode> and <EuiCode>value</EuiCode> are
+          strings. If you&apos;re using nodes, use the{' '}
+          <EuiCode>.eui-textTruncate</EuiCode>


Suggested change

<EuiCode>.eui-textTruncate</EuiCode>

<EuiCode>.eui-textTruncate</EuiCode>{' '}

cchaos · 2020-06-02T15:12:16Z

src-docs/src/views/expression/truncate.js

+  <div>
+    <p className="eui-textTruncate">.kibana_task_manager</p>
+    <p className="eui-textTruncate">kibana_sample_data_ecommerce</p>
+  </div>


Suggested change

<div>

<p className="eui-textTruncate">.kibana_task_manager</p>

<p className="eui-textTruncate">kibana_sample_data_ecommerce</p>

</div>

<>

<p className="eui-textTruncate">.kibana_task_manager</p>

<p className="eui-textTruncate">kibana_sample_data_ecommerce</p>

</>

thompsongl

Code LGTM 👍

kibanamachine · 2020-06-02T18:02:33Z

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

…ression (elastic#3467) * added prop * added test and updated example * add cl * added description to tooltip * renamed prop, turned into enum * example is function component * update snippet * make the example more like alerting * new styles * basic combobox working * more progress on example * added align right * removed truncate * renamed column prop * cleanup * updated tests and added handling of color * remove unneeded line * Review updates * update test * re-added textWrap prop * update cl * PR feedback Co-authored-by: cchaos <caroline.horn@elastic.co>

andreadelrio added 2 commits May 11, 2020 22:29

added truncate example, pending prop

c4d93cd

added prop

faaa6ad

andreadelrio force-pushed the expression-truncate branch from 06dd882 to 4adbc16 Compare May 13, 2020 02:26

added test and updated example

23dad35

andreadelrio force-pushed the expression-truncate branch from 4adbc16 to 23dad35 Compare May 13, 2020 02:51

andreadelrio added 2 commits May 12, 2020 19:53

add cl

f03e8fe

remove unneeded line

0f13450

andreadelrio changed the title ~~Add truncation to EuiExpression~~ Add truncate prop to EuiExpression May 13, 2020

added description to tooltip

1bf1d04

andreadelrio marked this pull request as ready for review May 13, 2020 04:51

andreadelrio requested review from cchaos and mdefazio May 13, 2020 04:52

fix conflict

60fa604

cchaos suggested changes May 13, 2020

View reviewed changes

src/components/expression/expression.tsx Outdated Show resolved Hide resolved

src/components/expression/_expression.scss Outdated Show resolved Hide resolved

src-docs/src/views/expression/truncate.tsx Outdated Show resolved Hide resolved

elizabetdev reviewed May 13, 2020

View reviewed changes

src-docs/src/views/expression/truncate.tsx Outdated Show resolved Hide resolved

andreadelrio added 3 commits May 13, 2020 14:26

renamed prop, turned into enum

efe0e00

example is function component

eb52280

Merge remote-tracking branch 'upstream/master' into expression-truncate

72b2918

andreadelrio changed the title ~~Add truncate prop to EuiExpression~~ Add textWrap prop to EuiExpression May 13, 2020

update snippet

dfc860a

andreadelrio requested a review from cchaos May 13, 2020 23:44

cchaos and others added 2 commits May 29, 2020 16:57

Review updates

4128407

Merge pull request #10 from cchaos/andreadelrio/expression-truncate

b606ecc

Review updates

andreadelrio added 2 commits May 31, 2020 22:25

added invalid example

5cc964a

update test

4e583f8

re-added textWrap prop =D

f6d39f2

andreadelrio changed the title ~~Add display, descriptionWidth and isInvalid props to EuiExpression~~ Add display, descriptionWidth, textWrap and isInvalid props to EuiExpression Jun 1, 2020

update cl

d15a4b4

andreadelrio force-pushed the expression-truncate branch from 4590e7a to d15a4b4 Compare June 1, 2020 18:21

cchaos suggested changes Jun 1, 2020

View reviewed changes

andreadelrio added 3 commits June 1, 2020 16:59

PR feedback

52dec78

tiny styling fix

3e21df3

update CL

e0209b0

mdefazio approved these changes Jun 2, 2020

View reviewed changes

andreadelrio requested a review from cchaos June 2, 2020 04:52

cchaos requested a review from thompsongl June 2, 2020 14:47

cchaos approved these changes Jun 2, 2020

View reviewed changes

thompsongl approved these changes Jun 2, 2020

View reviewed changes

nits

85b20b6

andreadelrio merged commit 7b00314 into elastic:master Jun 2, 2020

thompsongl mentioned this pull request Jun 3, 2020

Upgrade EUI to v24.1.0 elastic/kibana#68141

Merged

	<EuiCode>truncate</EuiCode> to the <EuiCode>textWrap</EuiCode> prop.
	<EuiCode language="ts">{'textWrap="truncate"'}</EuiCode>.

	<EuiCode>.eui-textTruncate</EuiCode>
	<EuiCode>.eui-textTruncate</EuiCode>{' '}

Conversation

andreadelrio commented May 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

kibanamachine commented May 12, 2020

Uh oh!

kibanamachine commented May 13, 2020

Uh oh!

kibanamachine commented May 13, 2020

Uh oh!

kibanamachine commented May 13, 2020

Uh oh!

mdefazio commented May 13, 2020

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented May 13, 2020

Uh oh!

andreadelrio commented May 13, 2020

Uh oh!

andreadelrio commented May 13, 2020

Uh oh!

mdefazio commented May 14, 2020

Uh oh!

kibanamachine commented May 29, 2020

Uh oh!

kibanamachine commented May 29, 2020

Uh oh!

andreadelrio commented Jun 1, 2020

Uh oh!

kibanamachine commented Jun 1, 2020

Uh oh!

cchaos commented Jun 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andreadelrio commented Jun 1, 2020

Uh oh!

andreadelrio commented Jun 1, 2020

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kibanamachine commented Jun 2, 2020

Uh oh!

mdefazio left a comment

Choose a reason for hiding this comment

Uh oh!

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

cchaos Jun 2, 2020

Choose a reason for hiding this comment

Uh oh!

cchaos Jun 2, 2020

Choose a reason for hiding this comment

Uh oh!

cchaos Jun 2, 2020

Choose a reason for hiding this comment

Uh oh!

cchaos Jun 2, 2020

Choose a reason for hiding this comment

Uh oh!

cchaos Jun 2, 2020

andreadelrio commented May 12, 2020 •

edited

Loading

cchaos commented Jun 1, 2020 •

edited

Loading