Into feature/range dropdowns #20
Conversation
| <EuiFormControlLayoutDelimited | ||
| fullWidth={fullWidth} | ||
| compressed={compressed} | ||
| startControl={minInput} | ||
| endControl={maxInput} | ||
| /> | ||
| } | ||
| isOpen={this.state.isPopoverOpen} | ||
| closePopover={this.closePopover} | ||
| // zIndex={popoverZIndex} | ||
| className={classNames('euiRange__popover', { | ||
| 'euiRange__popover--fullWidth': fullWidth, |
There was a problem hiding this comment.
This is something we do a lot with inputs, make them fullWidth. Perhaps we can just have EuiInputPopover accept a fullWidth prop to take care of that styling innately?
There was a problem hiding this comment.
So I did this, but it brings up another point: the full width popover looks weird with a full width input
There was a problem hiding this comment.
Yes, and no, really it would end up being up to the maker of the input, but a lot of times we see fullWidth being applied because the area they're putting the control in is adjustable so they want the control to increase with the user's adjusted width. The creator of the component though could put a max-width on it that's larger or smaller than the default form max width.
There was a problem hiding this comment.
Works for me. I think adding the fullWidth prop to this new component makes it more focused on form controls, which increases the argument for a new component (thread below)
| import { EuiFocusTrap } from '../focus_trap'; | ||
| import { EuiPopover, EuiPopoverPropTypes } from './popover'; | ||
|
|
||
| export const EuiInputPopover = ({ |
There was a problem hiding this comment.
I know we mostly just use this with inputs, but really all this component does is "attach" the popover to the triggering element and match the width. Perhaps we can make this more generic, mainly in the concern of naming and call it EuiPopoverAttached. Then it would probably change the prop input to anchor.
There was a problem hiding this comment.
Oh I see now, there is the option to "attach" the normal EuiPopover, but this new component ensures it matches the width...
There was a problem hiding this comment.
Do you foresee this being added to in an form control-specific way? Or does this seem like something that might stay generic?
There was a problem hiding this comment.
To be honest, I'm not sure right now.
As is, I'm running into difficulties capturing the necessary key press and focus events because of limited access to the underlying form components.
So as a general EuiPopoverAttached component this works well. But when coupled with having to discern events from form controls, it needs a bit more to accomplish a11y needs.
There was a problem hiding this comment.
I think I still prefer the move to a new component, rather than rely on configuration. The a11y problem doesn't really get any easier just using EuiPopover with an extra prop, anyway.
For the range case in particular, I think the current state of things is valid:
- click opens the popover for more clarity, and tab will eventually close it.
- keyboard nav in does not open the popover, but a keyboard savvy user still gets the affordance of a
numberinput.
Thoughts?
There was a problem hiding this comment.
All that to say, we'll see changes as future form controls want to uses this pattern. But a lot of that will be influenced by the broader popover discussion that's ongoing.
There was a problem hiding this comment.
Are you happy with where this is at currently then? I can merge it in if so.
There was a problem hiding this comment.
Yeah we can at least let others add opinions in the main thread
cchaos
force-pushed
the
into-feature/range-dropdowns
branch
from
1ee7727 to
9895abf
Compare
cchaos
force-pushed
the
into-feature/range-dropdowns
branch
from
34fdf47 to
b9937a5
Compare
thompsongl
force-pushed
the
into-feature/range-dropdowns
branch
from
0e6e508 to
536b9b9
Compare
|
Starting over |
Summary
Still need to figure out focus management, but at least you can get started looking at service and styles generally.