Discrete unit input by bevacqua · Pull Request #353 · elastic/eui

bevacqua · 2018-01-30T21:01:29Z

A new kind of input field that conforms to our design guidelines but allows the user to add or remove discrete values.

Usage example where this.state.discreteValues is ['a', 'b', 'c', 'd'], and insert/remove modify said state object via React's setState:

<EuiFieldDiscrete
  values={this.state.discreteValues}
  renderValue={value => value.toUpperCase()}
  onInsert={this.onDiscreteInsert}
  onRemove={this.onDiscreteRemove}
  placeholder="Type and press enter …" />

An isValid prop can be provided, and should return true only when an input string would be valid
There's also a parse prop you can provide, which should worry about taking the plain text user input and mapping it into whatever value we actually need for rendering and for our collection of discrete values
Note you can provide renderValue and return whatever you want to display each item

The input renders the list of values using the renderValue prop, or just casting them to strings.

On hover, items show a cross that signals they can be removed.

When clicking on the input, the entire discrete input gets focus styles, and not just the actual input element

The focus behavior is enabled thanks to a second component introduced in this PR, EuiFocusEmulator, meant to be used internally by EUI, which makes sure to transfer focus/disabled/invalid state and styles to the parent when the input gets one of those states.

Usage of <EuiFocusEmulator> is below. The <input /> doesn't need to be inside the emulator, but when the input is clicked, the <div> emitted by the emulator will get the styles that were meant for the input. Note that it should be a raw input, not a <EuiFieldText> or such, since the emulator is meant to be the target of our styling, so this component is meant mostly to be reused in cases like this where we have an actual input inside a component that's supposed to say "hey, this whole thing has focus", not just the input itself, which would look bad"

<EuiFocusEmulator getSource={ () => this.input }>
  <input ref={ el => { this.input = el } } />
</EuiFocusEmulator>

/cc @andrew-moldovan @gjones

bevacqua · 2018-01-30T21:06:33Z

Invalid state:

andrew-moldovan · 2018-01-30T21:12:55Z

Fancy! 🎉 🎉

cjcenizal

I've given a very cursory glance through this PR, but I'll have more time to look at it more closely tomorrow. From looking at the PR description, this looks really cool!

A couple things jumped out at me from my quick once-over. Also, can we add tests for these new components?

cjcenizal · 2018-01-30T21:32:39Z

src/components/form/field_discrete/_field_discrete.scss

+  padding: 0;
+}
+
+.euiFieldDiscrete-input {


For child elements, can we use a double underscore to concatenate the parts of the name? e.g. euiFieldDiscrete__input. We've been following the Kibana CSS style guide for our styles.

cjcenizal · 2018-01-30T21:33:49Z

src/components/form/field_discrete/field_discrete.js

+
+    const fieldClasses = classNames('euiFieldDiscrete', className);
+
+    const isInvalid = value !== `` && !isValid(value);


Why check against an empty string here? Shouldn't isValid be responsible for determining validity?

This is internal state, an empty value means the input doesn't have anything, so we don't accept "nothing" as valid input, but marking the field as invalid because its empty would be weird? I can see how the consumer might be the one who gets to decide that :)

cjcenizal · 2018-01-30T21:34:34Z

src/components/form/field_discrete/field_discrete.js

+    const { value } = this.state;
+    const { isValid, parse, onInsert } = this.props;
+
+    if (e.key === 'Enter') {


Can we import the keyCodes service and use it here?

cjcenizal · 2018-01-30T21:36:55Z

src/components/badge/badge.js

  accent: 'euiBadge--accent',
  warning: 'euiBadge--warning',
  danger: 'euiBadge--danger',
+  ghost: 'euiBadge--ghost',


Can we update the Badge page of the doc site with an example of this?

cjcenizal · 2018-01-30T21:37:16Z

src/components/form/_index.scss

+    inset 0 -2px 0 0 $euiColorPrimary;
+}
+
+@mixin euiFormControlStyleDisabled {


I love that you pulled these out!

cjcenizal

This is really cool Nico!! I love it. I have a few suggestions and some requests.

Tests

Would you mind adding Jest tests for the new components you've added?

Hover state

Currently, the hover state replaces the entire badge with the "x" icon. This makes for a tough UX for me because when I hover it I can't confirm what I'm deleting. I'm sure @snide and @cchaos can provide a strong suggestion. Off-hand, I imagine showing the "x" icon on the right side of the badge would work. Maybe keeping it permanently visible:

Long badge text

When entering long badge text, I was able to get the component to enter a broken visual state.

Maybe we should add a max-width to each badge and truncate the overflowing text? That's what they did for the example I provided in my previous point.

cjcenizal · 2018-01-31T21:53:00Z

src/components/form/focus_emulator/focus_emulator.js

+import React, { Component } from 'react';
+import classNames from 'classnames';
+
+export class EuiFocusEmulator extends Component {


Can we add an example of this to the doc site? Perhaps in its own example page.

And I would be out of character if I didn't suggest another name... 😄 "Emulator" confuses me... how about naming this EuiFieldProxy? Because it surfaces the visual cues of a field, driven by the "true field" beneath the surface. So it proxies the underlying field for visually communicating state to the user.

cjcenizal · 2018-01-31T22:04:45Z

src/components/form/focus_emulator/focus_emulator.js

+    this.setState({ hasFocus: false });
+  }
+
+  onClick = (...rest) => {


I'm not sure I understand the role this plays. Can you explain it for me?

Sure. Rather than assuming an e parameter and passing that to this.props.onClick, I call this.props.onClick(...rest) and I never have to worry about the API for onClick changing or adding parameters I didn't originally account for. Or so the theory goes

Ah I see that makes sense. Though it would be such a breaking change to the API that I am certain it would never happen, so maybe this is being overly defensive? Using ...rest instead of e makes this code look special so I can see other people being confused by it if they come across it. Can we either change the parameter to being the more conventional event, or add a comment to explain it the way you just did for me?

cjcenizal · 2018-01-31T22:42:09Z

src/components/form/field_discrete/field_discrete.js

+  EuiFocusEmulator
+} from '../focus_emulator';
+
+export class EuiFieldDiscrete extends Component {


Naming suggestion: how about EuiFieldBadges or EuiFieldTags? The dumber the better. This way people don't need to learn what "discrete" means in this context.

cjcenizal · 2018-01-31T22:44:44Z

src/components/form/focus_emulator/_focus_emulator.scss

+  @include euiFormControlStyleDisabled;
+}
+
+.euiFocusEmulator--invalid {


All three of these classes are currently formatted as modifiers, but they're actually states. Can we rename them?

.euiFocusEmulator-isFocused .euiFocusEmulator-isDisabled .euiFocusEmulator-isInvalid

cjcenizal · 2018-01-31T22:45:29Z

src/components/form/field_discrete/field_discrete.js

@@ -0,0 +1,150 @@
+import { noop, omit } from 'lodash';


We're removing our dependency upon lodash. Could you inline these functions? CC @uboness

cjcenizal · 2018-01-31T22:45:54Z

src-docs/src/views/form/form_controls.js

@@ -1,3 +1,5 @@
+import { without } from 'lodash';


We're removing our dependency upon lodash. Could you inline these?

That sounds great, what's the reasoning behind this? Maybe instead of making lodash a module non grato, we could switch to importing from the smaller bundles like lodash/without and so on?

We hit a problem where we were using a version of lodash incompatible with the one Kibana was using (addressed by #359). I think the best way to avoid problems like that in the future is by removing the dependency entirely... which seems viable/reasonable to me because we're mostly using some really simple lodash utilities, which should be simple to internalize and maintain. This way, lodash will no longer be a critical dependency for using EUI, which should simplify consumption.

What do you think of this approach? Are there modules in lodash you feel we really need, which we would have trouble maintaining?

I think this is a non-solution to that problem. We've hit so many problems due to the way we distribute EUI (build headaches, svg icons, now lodash, etc). As long as we refuse to bundle EUI into a drop-in thing that compiles whatever code it needs into a nicely self-contained bundle, we'll keep hitting these issues.

Sure, not using lodash wouldn't be such a huge deal, but it doesn't fix the underlying problem that just keeps spitting out issues for us to deal with.

In any case, there's enough stuff using lodash today that I'd defer the removal of lodash calls to a new PR unless it's already being actively worked on

Are there modules in lodash you feel we really need, which we would have trouble maintaining?

Well there's a ton of tiny utilities in it, obviously. Some are trivial to reimplement, some aren't that trivial. None are a huge concern, obviously, this being a utility library, but why reinvent the wheel when there's this large set of finely tuned utility functions at our disposal

In any case, there's enough stuff using lodash today that I'd defer the removal of lodash calls to a new PR unless it's already being actively worked on

SGTM

cjcenizal · 2018-01-31T22:47:35Z

src/components/form/field_discrete/field_discrete.js

+      }
+    };
+
+    const otherProps = omit(rest, 'parse', 'onInsert');


Could you explain this bit to me?

I'm passing otherProps to the input (or your suggestion to pass them to the parent div, not sure which is better). parse and onInsert aren't using on render, so I need to exclude them from the rest bag even after destructuring props that we do use, otherwise they'd fall through to the child component

Oh I see! Thanks, that makes sense. Could I suggest pulling them out in the original destructuring assignment on line 34 and using // eslint-disable-line no-unused-vars? I believe this is how we've been doing this kind of thing elsewhere in the codebase.

cjcenizal · 2018-01-31T22:49:24Z

src/components/form/field_discrete/field_discrete.js

+      onRemove,
+      className,
+      disabled,
+      ...rest


Typically we pass ...rest through to some part of the rendered DOM, so that the consumer can provide arbitrary HTML attributes. How about on line 71?

See previous comment, was already passing ...otherProps to the <input />

cjcenizal · 2018-01-31T22:49:53Z

src/components/form/field_discrete/field_discrete.js

+          disabled={disabled}
+          invalid={isInvalid}
+        >
+          <div className={fieldClasses}>


My suggestion is:

<div className={fieldClasses} {...rest}>

cjcenizal · 2018-01-31T22:50:45Z

src/components/form/field_discrete/field_discrete.js

+                    color="ghost"
+                    className="euiFieldDiscrete-value"
+                    onClick={() => onRemove(value)}
+                  >


Would you mind adding a data-test-subj="fieldProxyBadge" attribute to this element so we can query it in our tests?

cjcenizal

Had a couple more thoughts about accessibility.

cjcenizal · 2018-01-31T23:00:00Z

src/components/form/field_discrete/field_discrete.js

+                    key={hash(value)}
+                    color="ghost"
+                    className="euiFieldDiscrete-value"
+                    onClick={() => onRemove(value)}


@timroes How do you think we should support screen reader accessibility for this component? I think it might make sense to pull aria-labelledby off the props and apply it to each badge.

@snide I think we need to solve the problem of clickable badges, e.g. by updating EuiBadge to support clicking on them or creating a new component for this role. Currently this component isn't keyboard-accessible because we didn't design for this use case.

I think @cchaos is coming up with something awesome for this FYI.

cjcenizal · 2018-01-31T23:03:50Z

src/components/form/field_discrete/field_discrete.js

+  fullWidth: false,
+  isLoading: false,
+  isValid: () => true,
+  renderValue: value => String(value),


I think we should create an individual example page for this component. Then we can add examples of how to use this renderValue, parse, and isValid.

bevacqua · 2018-02-02T14:15:24Z

New hover state:

I'm kind of strongly against adding the remove icon inline because it's not something you want to see all the time, but once you hover around the badges it becomes clear you can remove them by clicking

bevacqua · 2018-02-02T14:49:13Z

Fixed broken CSS

bevacqua · 2018-02-02T14:56:17Z

Naturally validation probably should do away with these ridic. long badge values in real world use cases

cchaos · 2018-02-02T15:58:49Z

Hey @bevacqua , I can help you a bit on the UI for this. I have a couple questions:

Is deletion the only action you can perform when clicking on a tag?
Would you consider truncating a really long badge name but ensuring that it has a title so on long hover they can view the whole name? The multi-line badge breaks the convention of a "pill" style and seems a bit out of place.
Do the badge names have to be in all caps? I would think that if a user types in a combination of capitals and lowercase letters, we should respect that especially in cases where it might include an acronym.

cchaos · 2018-02-02T16:06:01Z

src-docs/src/views/form/form_controls.js

+          renderValue={value => value.toUpperCase()}
+          onInsert={this.onDiscreteInsert}
+          onRemove={this.onDiscreteRemove}
+          placeholder="Type and press enter …"


I would remove the ellipses. We have some writing guidelines on the usage of punctuation, in particular ellipses: https://elastic.github.io/eui/#/writing (under Punctuation).

I also noticed that in your screenshots it stuck around once someone was typing. This seems a bit of odd behavior.

I also noticed that in your screenshots it stuck around once someone was typing. This seems a bit of odd behavior.

I just typed some more … to drive you mad, but they don't stick around 😅

cjcenizal · 2018-02-02T16:45:03Z

src/components/form/focus_emulator/focus_emulator.js

+        className={classes}
+        disabled={disabled}
+        invalid={invalid}
+        onClick={this.onClick}


I think it makes sense to look again at this in the next iteration, so this comment is more a note for myself.... this onClick is currently inaccessible, which can be solved in a number of ways. But I think the appropriate solution will become apparent after we address the accessibility of the pills.

bevacqua · 2018-02-02T17:53:59Z

Demo page

* Move react-color from devDependencies to dependencies. * Update CHANGELOG.

- created a `predicate` module that exports all predicate function (e.g. `isString`, `isFunction`,...) - created `isNil` predicate - `sortable_properties` no longer uses lodash `sortBy` and uses our comparators instaed this change should enable using tables in Kibana.

…emo missing.

bevacqua · 2018-02-02T18:06:57Z

Would you consider truncating a really long badge name but ensuring that it has a title so on long hover they can view the whole name? The multi-line badge breaks the convention of a "pill" style and seems a bit out of place.

I would, but I wouldn't want the component to be that smart. If I need this, I'll do it as a consumer, imo.

Do the badge names have to be in all caps? I would think that if a user types in a combination of capitals and lowercase letters, we should respect that especially in cases where it might include an acronym.

They can be rendered however the consumer chooses, it's completely up to the consumer

bevacqua · 2018-02-02T18:08:12Z

Is deletion the only action you can perform when clicking on a tag?

No, but it's not a problem anymore now that the delete action shows up top right instead of overlaying

cjcenizal · 2018-02-02T18:49:15Z

@bevacqua On the Kibana side, we've found that we're able to make faster and steadier progress using EUI if we bake some of our design rules into our components. This has meant that recently we've started approaching our component design expecting them to be more opinionated than we have in the past. What has the experience regarding opinionated components been like on your side? Have you had cases where greater flexibility has been advantageous?

In terms of this component, I'd like to propose this solution, which I think will strike the right balance between defaulting to an opinionated visual state (and thereby a simpler interface) while still offering an escape hatch for consumers who want greater control:

Let's treat the design @cchaos comes up with as the default and most common use cases.
Let's adjust the interface so that values is actually badges or items -- something which does not imply there is a value involved, but instead is something that just gets rendered. It might be best to leverage children in this case.
This prop should be array of nodes (so that either strings or elements will render just fine).
If it's a string, let's get opinionated about truncating it with an ellipsis automatically. If you can find us a use case for a multi-line pill then I could change my mind, but right now I think it's a safe rule to state we should never do this.
Let's not be opinionated about casing -- let's make the consumer responsible for this, since appropriate casing could vary based on what the content is (e.g. US state names will need all caps, as would my name, but lower-casing makes more sense for common nouns).
If it's an element, we'll just wrap it in the badge so you can have your emojis.
Point 5 also implies that we should drop the renderContent and parsing props. I think these make more sense as the responsibility of the consumer and not this component.

Thoughts?

bevacqua · 2018-02-02T20:13:19Z

This is getting out of hand, unless we can move those requests to a future iteration I'll need to pull this code into Cloud and come back to this PR in the future.

bevacqua · 2018-02-02T20:22:02Z

Closing for now

cjcenizal · 2018-02-02T20:24:48Z

Sorry, Nico! Don't want to hold you up, but I also want to be a good shepherd to EUI. :) Feel free to put this on hold if you have to -- we'll probably need a little time to update EuiBadge to support our new design anyway.

Not that it matters, but I think most of my comments mostly entailed moving the parsing code out of the component into the consumer, which seems like mostly cut-and-pasting...? Or not, I could be wrong.

bevacqua added 2 commits January 30, 2018 17:44

discrete unit input

56ed9bf

ghost badge

ab3ad2b

bevacqua requested review from cjcenizal and snide January 30, 2018 21:01

transfer props

fbac96e

cjcenizal reviewed Jan 30, 2018

View reviewed changes

cjcenizal suggested changes Jan 31, 2018

View reviewed changes

cjcenizal reviewed Jan 31, 2018

View reviewed changes

cchaos reviewed Feb 2, 2018

View reviewed changes

cjcenizal reviewed Feb 2, 2018

View reviewed changes

cjcenizal and others added 10 commits February 2, 2018 15:03

Move react-color from devDependencies to dependencies. (elastic#354)

513e390

* Move react-color from devDependencies to dependencies. * Update CHANGELOG.

Update CHANGELOG for 0.0.16.

728e2bf

0.0.16

daf4e9a

Updated documentation.

96391bd

Update CHANGELOG for 0.0.17.

066909d

Update CHANGELOG.

1132e75

0.0.17

322ebc8

Updated documentation.

280eaea

code review follows CJ comments, demo for field badges. tests/proxy d…

4354899

…emo missing.

bevacqua added 3 commits February 2, 2018 15:14

merge master

024dac6

fixes because dark mode

53c223e

only mess with input if it's actually a children of proxy

0d41435

bevacqua closed this Feb 2, 2018

cjcenizal mentioned this pull request Mar 25, 2018

EuiComboBox #567

Merged

20 tasks


		const fieldClasses = classNames('euiFieldDiscrete', className);

		const isInvalid = value !== `` && !isValid(value);

Conversation

bevacqua commented Jan 30, 2018

Uh oh!

bevacqua commented Jan 30, 2018

Uh oh!

andrew-moldovan commented Jan 30, 2018

Uh oh!

cjcenizal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjcenizal left a comment

Choose a reason for hiding this comment

Tests

Hover state

Long badge text

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjcenizal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

cjcenizal Jan 31, 2018 •

edited

Loading

cjcenizal Feb 2, 2018 •

edited

Loading

cjcenizal commented Feb 2, 2018 •

edited

Loading