[Visual Refresh] Add EuiFormControlButton component #9006
Conversation
mgadewoll
force-pushed
the
forms/270-input-buttons
branch
from
703f629 to
45c37cd
Compare
|
LGTM 🚀 |
| <EuiFormControlLayout {...args} clear={{ onClick: handleOnClear }}> | ||
| <EuiInputPopover | ||
| ownFocus | ||
| input={formControlButton} |
There was a problem hiding this comment.
[question] 💭 this input={...Button} felt weird at first sight, but I guess it represents the whole point of this new component — still I was wondering: is there anything we could/should do to ensure this new button works as well with regular EuiPopover.button?
There was a problem hiding this comment.
is there anything we could/should do to ensure this new button works as well with regular EuiPopover.button?
@acstll It already works as expected also with EuiPopover 🙂
The screen recoding shows the following:
- EuiFormControlButton with EuiPopover within EuiFormControlLayout (requires EuiPopover to have
display: 'block') - EuiFormControlButton with EuiPopover
<EuiFormControlLayout {...args} clear={{ onClick: handleOnClear }}>
<EuiPopover
button={formControlButton}
isOpen={isPopoverOpen}
closePopover={() => {}}
display="block"
>
content
</EuiPopover>
</EuiFormControlLayout>
<EuiPopover
isOpen={isPopoverOpen}
button={formControlButton}
closePopover={() => {}}
>
content
</EuiPopover>Screen.Recording.2025-09-30.at.12.05.21.mov
| <EuiInputPopover | ||
| ownFocus | ||
| input={( |
There was a problem hiding this comment.
following up on the previous comment, would it make sense to add a sentence (admonition?) suggesting the usage of EuiInputPopover over EuiPopover?
There was a problem hiding this comment.
I'm not sure that we need to be that specific. In theory EuiFormControlButton could be used as any kind of trigger, also for EuiPopover.
| it('is rendered', () => { | ||
| const { getByTestSubject } = render( | ||
| <EuiFormControlButton {...defaultProps} /> | ||
| ); | ||
|
|
||
| expect( | ||
| getByTestSubject(defaultProps['data-test-subj']) | ||
| ).toHaveTextContent(defaultProps.value); | ||
| }); |
There was a problem hiding this comment.
[question] did you think about the <button>'s value attribute? would it make sense to also render value as value={value} in the button itself? I really don't think it will ever be needed (thinking about Kibana), but I didn't want to not (at least) bring it up 😄
There was a problem hiding this comment.
That's a good suggestion. Since it's a native attribute, we should pass it along even if it's most likely not gonna be used. 👍
Updated in ba522fc
|
ℹ️ Moving this PR back to draft for now, as there have been some discussions around ongoing updates in Kibana Dashboards that might have an effect on the need/requirement. Once we know more, we can update/proceed. |
ℹ️ Dashboards confirmed that our update can proceed and the adjustments needed for the new project would be done on Kibana side. |
- difference is 1px due to flex centering instead of content centering
mgadewoll
force-pushed
the
forms/270-input-buttons
branch
from
45c37cd to
ba522fc
Compare
💚 Build SucceededHistory
cc @mgadewoll |
💚 Build Succeeded
History
cc @mgadewoll |
Summary
closes https://github.com/elastic/eui-private/issues/270
This PR adds a new EUI component:
EuiFormControlButton.This component is meant to be used in form layouts (within
EuiFormControlLayout) when a button element is needed that maintains the visual relation to form elements.EuiFormControlButtoncombines a base set of props fromEuiButtonEmptywith form related props to ensure it behaves like an input element withinEuiFormControlLayout.Changes
EuiFormControlButton(component, tests, stories and documentation)EuiSuperDatePickerto useEuiFormControlButtonwhenshowPrettyDurationIdeation
Initially, the idea was to provide
EuiFormControlButtonand a specificEuiFormFilterButtoncomponent which would have inherited functionality fromEuiFilterButtonand styling fromEuiFormControlButton.EuiFormFilterButtonwould have been a direct replacement for the existing usage ofEuiFilterButtoninsideEuiFormControlLayoutbut creating a component for a single use case seemed excessive.Instead, the expected usage now is to use the more generic
EuiFormControlButtonand handle the filter badge output manually by passing theEuiBadgeNotificationas content (usage, example). It's slightly less convenient but seems more reasonable for the time being. If more use cases may appear, we could reconsider adding a dedicated component.Why are we making this change?
💅 UI consistency: Adding this component solves a design and usage requirement on consumer side. Currently consumers are using regular buttons inside form layouts which results in unwanted visual output.
Screenshots #
EuiFormControlButton
EuiFormControlButton within EuiFormControlLayout
Kibana Dashboard control
EuiSuperDatePicker
Impact to users
🟢 There are no updates required by consumers.
ℹ️⚠️ We want to update the existing use case on Kibana. The update was prepared here
QA
💻 The changes have additionally been added and deployed in Kibana (instance, credentials)
EuiFormControlButton(Storybook) and e.g.EuiFieldText(Storybook) and verify that they look alike (in terms of colors, text, border, states etc)EuiFormControlButtoninsideEuiFormControlLayout(Storybook) works as expected (form layout props render correctly)General checklist
@defaultif default values are missing) and playground togglesChecked Code Sandbox works for any docs examplesIf applicable, added the breaking change issue label (and filled out the breaking change checklist)If applicable, file an issue to update EUI's Figma library with any corresponding UI changes. (This is an internal repo, if you are external to Elastic, ask a maintainer to submit this request)