-
Notifications
You must be signed in to change notification settings - Fork 843
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
EuiSuperSelect #921
EuiSuperSelect #921
Conversation
@chandlerprall Do you think you could help out with the JS stuff for this one? |
Yes, we'll need it. Though I think it should just come from |
@cchaos yeah, no problem! I can get to it later today / tomorrow |
@chandlerprall, Any chance you will be able to get back to this this week? |
@cchaos yes! Sorry, fell off my radar. |
@chandlerprall Sorry, I merged your width PR in without testing. I noticed that when the popover is closing it doesn't maintain the determined width: https://d.pr/free/v/xzqbKp |
@chandlerprall thoughts on the above? |
3679b4c
to
4942a1d
Compare
Ok, this is finally no longer a WIP. Ready for review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Read through the code, tested both the new select and the adjusted context menu in IE11 and Chrome. Also went and built one to test it out.
Comments are pretty minor. Feel free to merge on your time.
I'd do three lines for your changelog. One for the new component, one for the popover change (hasArrow) and one for the context menu layout stuff.
|
||
render() { | ||
return ( | ||
<Fragment> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can prolly yank this fragment. Look like there's only one child?
|
||
render() { | ||
return ( | ||
<Fragment> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again here
text: ( | ||
<div> | ||
<p> | ||
This is a simple replacement component for <EuiCode>EuiSelect</EuiCode> if you |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nice if over on the EuiSelect
docs we back to this EuiSuperSelect
page as well. Guessing a lot of people would find it from there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
@@ -34,3 +34,19 @@ | |||
.euiContextMenuItem__arrow { | |||
align-self: flex-end; | |||
} | |||
|
|||
.euiContextMenu__itemLayout { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note, that I would spend 5 minutes on this. I don't think it's a big deal, was just something small i noticed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole popover is fuzzy (even the text). I don't think it's related to the drop shadow stuff. I'll look into it though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could very well be from fractional numbers on popover positioning. LMK if that's the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even with decimal placement it is fine:
Though, @chandlerprall maybe the positioning service should just go ahead and round to a whole number?
} | ||
} | ||
|
||
.euiSuperSelect__popoverPanel[class*="bottom"] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do? If it's anticipating a particular class, then maybe it makes more sense to use that specific class name(s) here so it's easier to find the reference if you change that class name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So instead of relying on a full classname that is created by a different component like .euiPopover__panel-bottom
, all I care about is the direction portion bottom
. So if the prefix to the class name ever changes, it won't affect this component that depends on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's important to remember we're also writing this codebase for everybody (Kibana team at large and community contributors)... so we should think about the point of view coming into this code and trying to figure out what it does so that they can either debug something or submit a PR. If someone comes to the super select and reads this line, they need to reverse engineer how the super select works in conjunction with the popover to understand what classes it could be interacting with. This line gives the reader a clue, and then the reader needs to spend time figuring it out.
But if it was just .euiSuperSelect__popoverPanel.euiPopover__panel-bottom
then the line tells the reader exactly what it does, and they can focus their time on their PR rather than trying to decipher the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think your concern is about needing to "reverse engineer" than just now knowing what that selector means. This type of selector is a very common one and we shouldn't need to be explicit (and therefore too dependent) for those that just aren't as CSS knowledgable. Its a quick lookup and you can easily see the result or debug in the browser's dev tools.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is worth blocking on, so I'm OK with merging this PR without this change. But if you want to know more about my motivations, here's a great article on the importance of writing code so it can be understood (not saying that you don't care about this already, but I just thought it's worth sharing).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I'll do is just add a comment about where that's coming from so then at least it's understandable.
@@ -0,0 +1,3 @@ | |||
.euiTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming this isn't supposed to be here?
e.preventDefault(); | ||
e.stopPropagation(); | ||
this.shiftFocus(SHIFT_BACK); | ||
} else if (e.keyCode === keyCodes.DOWN) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works great, love that it goes back to the top. I'd also add a key event for "space" that does the same as down so that it mirrors the behavior of a HTML select.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does SPACE
not work as expected for you? For me, if the select box is focused but not open SPACE
opens the options list, if an option is focused SPACE
selects that option and closes the dropdown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SPACE
works for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, sorry, chatted this in slack. Was a bad report on my side. This works as expected.
src/components/popover/_popover.scss
Outdated
@@ -131,6 +131,11 @@ | |||
} | |||
} | |||
} | |||
|
|||
|
|||
&.euiPopover__panel-noArrow .euiPopover__panel__arrow { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Element of an element? Maybe just make this something unique?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, should have been more direct.
.euiPopover__panel__arrow
I think we've tried to avoid using underscores twice in a single selector. My read of this is that arrow is an element of the element panel of the block popover.
Should it just be....
euiPopover__panelArrow
as the element of popover instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that was leftover from another PR, but I can change it here
Wow, the globalToastList tests failed... now THAT'S weird.
|
Retest |
Jenkins test this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏 This is awesome! I tested it in the browser and it works great. Code looks great overall, I just had a few suggestions with one big suggested change to the interface.
I did run into a problem with screen-reader accessibility. When I tab to the super select, VoiceOver on OS X doesn’t read the selected option. When I tab to a regular select
element, the selected option is read aloud first, and then the element itself is identified. Can we fix this so the behaviors are identical/similar?
/> | ||
); | ||
|
||
const items = options.map((option, index) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can support passing through props here (see next comment):
const items = options.map((option, index) => {
const {
value,
dropdownDisplay,
inputDisplay,
...optionRest
} = option;
return (
<EuiContextMenuItem
key={index}
className={itemClasses}
icon={valueOfSelected === value ? 'check' : 'empty'}
onClick={() => this.itemClicked(value)}
onKeyDown={this.onItemKeyDown}
layoutAlign={itemLayoutAlign}
buttonRef={node => this.setItemNode(node, index)}
style={{ width: this.state.menuWidth }}
{ ...optionRest }
>
{dropdownDisplay || inputDisplay}
</EuiContextMenuItem>
);
});
constructor(props) { | ||
super(props); | ||
|
||
this.options = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add an example of disabled
options and the application of pass-through props?
this.options = [
{
value: 'option_one',
inputDisplay: 'Option one',
disabled: true,
'data-test-subj': 'option one',
},
{
value: 'option_two',
inputDisplay: 'Option two',
},
{
value: 'option_three',
inputDisplay: 'Option three has a super long text to see if it will truncate or what',
},
];
It looks like unexpected props aren't passed through.
} | ||
|
||
onItemKeyDown = e => { | ||
if (e.keyCode === keyCodes.ESCAPE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switch
might be more appropriate here since all of the conditions assert against the same variable:
const { ESCAPE, TAB, UP, DOWN } = keyCodes;
// in all cases, close the popover and prevent ancestors from handling
switch (e.keyCode) {
case ESCAPE:
e.preventDefault();
e.stopPropagation();
this.closePopover();
break;
case TAB:
// no-op
e.preventDefault();
e.stopPropagation();
break;
case UP:
e.preventDefault();
e.stopPropagation();
this.shiftFocus(SHIFT_BACK);
break;
case DOWN:
e.preventDefault();
e.stopPropagation();
this.shiftFocus(SHIFT_FORWARD);
break;
}
* 2. Ensure the descenders don't get cut off | ||
*/ | ||
|
||
.euiSuperSelectControl { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's an extra space at the beginning of this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, my IDE doesn't like those comment blocks so anything afterward it tries to align to the comment.
} | ||
} | ||
|
||
.euiSuperSelect__popoverPanel[class*="bottom"] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do? If it's anticipating a particular class, then maybe it makes more sense to use that specific class name(s) here so it's easier to find the reference if you change that class name?
{ | ||
value: 'option_one', | ||
inputDisplay: 'Option one', | ||
dropdownDisplay: ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of putting inputDisplay
and dropdownDisplay
on each option, I think it might make more sense to treat these options as data-only, without any responsibility for rendering. Then we could provide props to the EuiSuperSelect
instance itself to render the input and the dropdown. This will allow these options to be defined by an API response without requiring us to then decorate each option. This is similar to how EuiComboBox
allows you customize the way each option is rendered (https://github.com/elastic/eui/blob/master/src-docs/src/views/combo_box/render_option.js#L117).
I'm thinking along these lines:
const healthToColorMap = {
0: 'warning',
1: 'subdued',
2: 'danger',
};
const healthToNameMap = {
0: 'Warning',
1: 'Minor',
2: 'Critical',
};
export default class extends Component {
constructor(props) {
super(props);
this.options = [
{
value: 'option_one',
// You can put anything you want inside of data.
data: {
name: 'Option one',
description: 'Has a short description giving more detail to the option.',
health: 0,
},
},
{
value: 'option_two',
data: {
name: 'Option two',
description: 'Has a short description giving more detail to the option.',
health: 1,
},
},
{
value: 'option_three',
data: {
name: 'Option three',
description: 'Has a short description giving more detail to the option.',
health: 2,
},
},
];
this.state = {
value: this.options[1].value,
};
}
onChange = (value) => {
this.setState({
value: value,
});
};
renderOption = option => {
const { name, description, health } = option.data;
return (
<Fragment>
<strong>{name}</strong>
<EuiHealth color={healthToColorMap[health]} style={{ lineHeight: 'inherit' }}>
healthToNameMap[health]
</EuiHealth>
<EuiSpacer size="xs" />
<EuiText size="s" color="subdued">
<p className="euiTextColor--subdued">{description}</p>
</EuiText>
</Fragment>
);
};
renderSelectedOption = option => {
return option.data.name;
};
render() {
return (
<Fragment>
<EuiSuperSelect
options={this.options}
valueOfSelected={this.state.value}
onChange={this.onChange}
itemLayoutAlign="top"
renderOption={this.renderOption}
renderSelectedOption={this.renderSelectedOption}
hasDividers
aria-label="Use aria labels when no actual label is in use"
/>
</Fragment>
);
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's valuable to have render-purposeful show this in the select box and show this in the drop down values. However, I would agree that also allowing a data
param to help track the represented data would be helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My worry with this option, is that each option is no longer customizable, unless you add a bunch of if
statements in the render methods. You could still do a similar thing with the current mapping by using the render methods as calls within the options declaration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the flexibility is nice in the way its set up now. I see us switching things out more from item to item with this one than the combobox.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My worry with this option, is that each option is no longer customizable
Do we have an example use case where each option is completely different? Do you think this will be a common requirement? I think we should optimize for the common use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it is right now, no. But it can be altered down the road to support this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @cchaos though I was more thinking from the technical perspective, e.g. is there a challenge with decorating hundreds (or thousands) of options with inputDisplay
and dropdownDisplay
properties, would it play well with react-virtualized
, etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cjcenizal That screen looks like EuiFilterGroup, which has a current implementation within our table / search components (which you can see on those pages respectively). It even adds search once it gets beyond 10 items. Will that not cover what you need? I don't know about it's scaling abilities, but it's a 1-1 design against the screen you showed, so my guess is that's the component we'd update further.
By design EuiSuperSelect should not have search in it and is really just a stylized select of choices. We have combo box and the filter group stuff for the pattern you describe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@snide Good call! Yes that could work, but it needs to be broken out into its own component. Looks like the table example you shared has it baked into its interface via the search
prop, and the standalone EuiFilterGroup example is just using a hand-rolled popover.
If we were to break it out into its own component, then at first glance it looks like the only difference between it and EuiSuperSelect would be that one has search and the other doesn't.
<EuiSuperSelectControl | ||
options={[ | ||
{ value: '1', inputDisplay: 'Option #1', disabled: false }, | ||
{ value: '2', inputDisplay: 'Option #2', disabled: true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add tests for passing className
and data-test-subj
to the options?
@cchaos any thoughts here? It seems to me that to get similar Mac VoiceOver results we'd want to integrate an actual select box (which opens another issue, the |
I think we can probably solve it with some onFocus announcements. Specifically i don't think it matters if it's an actual select vs. a collection of buttons, and a role="group" with a proceeding announcement should work fine. |
I'll defer to whatever choice makes it work. |
@cchaos I'll give it a shot and submit your a PR. |
6c72117
to
8eb1e8b
Compare
@chandlerprall All there is now, for addressing feedback, revolves around the discussion of needing to provide |
This reverts commit e8c3fa0.
274 super select pr feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found a couple small things -- the only major blocker are the tests. The component navigation sounds good on VoiceOver. Great work @snide!
|
||
this.state = { | ||
value: this.options[1].value, | ||
inputDisplay: this.options[1].inputDisplay, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this isn't being consumed anywhere, so I think we can remove it?
onChange = (value) => { | ||
this.setState({ | ||
value: value, | ||
inputDisplay: this.options.inputDisplay, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we can simplify this to:
this.setState({ value });
.toMatchSnapshot(); | ||
}); | ||
|
||
describe('props', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to mount the components in these tests and then click to open them or manually set the open state. We can then use the takeMountedSnapshot
utility to get a snapshot of it. Otherwise the snapshots will just consist of closed super selects. Here's an example of a similar test with the combo box: https://github.com/elastic/eui/blob/master/src/components/combo_box/combo_box.test.js#L75
Update EuiSuperSelect unit tests to open the options before snapshot
@cjcenizal can you verify the tests @chandlerprall merged in are what you'd expect. i think this one is good to go? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests look good, just had one suggestion about a test we might not need.
}); | ||
|
||
describe('props', () => { | ||
test('options are rendered', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't look like this test is effective since the options list isn't open in the snapshot. But I also think it might not be necessary, since you're testing it indirectly in the super select test suite.
Addresses #274 , fixes #978
This creates a custom
EuiPopover
implementation that styles the triggering button to look like aEuiSelect
and populates a list ofEuiContextMenuItem
s based on anoptions
object prop.The issue right now is that it still behaves like a popover and not a select. Things that need help:
EuiComboBox
EuiComboBox
or whatever the fix is going to be forEuiPopover