Upgrade EUI to v77.2.2 #155208
Upgrade EUI to v77.2.2 #155208
Conversation
1Copenut
added
release_note:skip
cee-chen
added
v8.9.0
and removed
backport:skip
cee-chen
force-pushed
the
feature/eui-v77.2.0
branch
from
d2b2a56
to
a708c48
Compare
|
Pinging @elastic/eui-team (EUI) |
botelastic
bot
added
the
Team:Uptime - DEPRECATED
|
Pinging @elastic/uptime (Team:uptime) |
|
There are some signifiant changes to EuiFieldNumber that are not well advertised. An invalid indicator is displayed for any EuiFieldNumber that accepts a decimal and does not also set
|
| @@ -90,6 +90,7 @@ export class DecimalDegreesForm extends Component<Props, State> { | |||
| onChange={this._onLatChange} | |||
| isInvalid={isLatInvalid} | |||
| data-test-subj="latitudeInput" | |||
| step="any" // Browsers will validate decimals as invalid otherwise | |||
There was a problem hiding this comment.
There are other cases where EuiFieldNumber accepts decimals and step is not set. How should these be addressed?
There was a problem hiding this comment.
Our recommendation would be for your decimal usages to set step="any": elastic/eui#5327
There are some signifiant changes to EuiFieldNumber that are not well advertised. An invalid indicator is displayed for any EuiFieldNumber that accepts a decimal and does not also set step="any".
Just to be clear, and so we're on the same page about this behavior - EuiFieldNumber is not doing anything to the default browser level ValidityState API for decimals inputs and the step prop.
Browsers are the ones defaulting step to non-decimal 1, not EUI. You can see this default behavior in this CodeSandbox when you type in 0.5 into a raw <input type="number"> with nothing but :invalid styling on it. You can also see in this example that adding step="any" (again, a native browser/HTML API, not EUI's) to the native input removes the :invalid styling.
So for all plugins in Kibana now noticing this behavior, it was already happening prior to this release/upgrade. EUI was already setting a red underline via the :invalid CSS selector on all form control elements, which is visible once the blue focus underline is not present. The browser-level tooltip error message was also already occurring, users likely simply weren't hovering over it to see it (or in some browsers, e.g. Safari, tooltips are less evident than e.g., Firefox).
All this change does is add an extra more visible icon when the native browser ValidityState flags something as :invalid.
So to summarize:
- If you're seeing this issue now, you already had it before, the icon just made it more obvious
- Devs are interacting with
EuiFieldNumberprops/attributes the exact same way you would interact with a<input type="number">field - The solution on all levels (EUI usage and native HTML usage) is for those implementing a number field to set
step="any"on fields where you expect decimals.
There is an argument to be made here for EuiFieldNumber to default step to "any" on your behalf to make this less of a manual process for consumers. In general, EUI prefers to stick to native/browser behavior as closely as possible, but there are absolutely exceptions to be made for things like accessibility, common pain points, and developer experience. We'd be super open to hearing if you'd prefer that to be the path we take.
There was a problem hiding this comment.
Just to be clear, and so we're on the same page about this behavior - EuiFieldNumber is not doing anything to the default browser level ValidityState API for decimals inputs and the step prop.
understood
If you're seeing this issue now, you already had it before, the icon just made it more obvious
This is what concerns me. I don't think this change has been given the visibility it deserves. The icon is now visible and may confuse users as the input now displays an error icon. I just want to make sure teams understand the implications and take the time to audit their usage of EuiFieldNumber. Otherwise, there are going to be a lot of broken looking numeric inputs.
I have created #156540 to track the issue for Maps.
There was a problem hiding this comment.
Gotcha. I think there's a few options we can take here:
- Add a notice to the Kibana newsletter and EUI newsletter to try and flag this behavior more and get folks to update before 8.9 FF
- As I mentioned above, I think there's an argument to be made here for defaulting
EuiFieldNumbertostep="any"- this would remove any work needed on Kibana's part. Do you have any thoughts / preference on this?
There was a problem hiding this comment.
As I mentioned above, I think there's an argument to be made here for defaulting EuiFieldNumber to step="any" - this would remove any work needed on Kibana's part. Do you have any thoughts / preference on this?
I am not sure if that would create more problems than it solves. I think you should expect consumers to properly set step and not rely on defaults
There was a problem hiding this comment.
Thanks Nathan - that's a super helpful point! We'll make sure to get this change added to the Kibana newsletter for awareness.
There was a problem hiding this comment.
Just wanted to follow up on this thread - after having drilled down into the default browser behavior a bit more, we realized that the reason why this behavior wasn't obvious to users & devs beforehand is because when step is undefined, browsers set :invalid state while typing but then set :valid after the input is blurred. Our focus styles (blue underline) were always taking precedence over invalid styles (red underline), and then on blur, both were removed. The icon, as mentioned, is what what flags the mis-alignment in what's happening vs. what's shown to the user.
After conferring with our designers, EUI has decided to go with defaulting to step="any" on EuiFieldNumber - we consider it to be a UX improvement without significant downsides over the arguably unintuitive default browser behavior. See elastic/eui#6760 for more details, or feel free to leave us comments there.
Thanks again for flagging this @nreese, it's seriously much appreciated.
| @@ -832,8 +832,9 @@ export default class QueryStringInputUI extends PureComponent<QueryStringInputPr | |||
| > | |||
| {this.forwardNewValueIfNeeded(this.getQueryString())} | |||
| </EuiTextArea> | |||
| {/* EUI TODO: This will need to be fixed before the Emotion conversion */} | |||
There was a problem hiding this comment.
This component is our EUI Sass-generated classNames directly instead of EUI components (i.e., className="euiFormControlLayoutIcons" vs. <EuiFormControlLayout>).
In general, we strongly advise against this usage of EUI - but specifically, once our form components are converted to Emotion, said classNames will no longer have any meaningful CSS attached to them and the visual appearance of this icon will break again.
Part of why this custom implementation was added was (I assume) because EuiTextArea doesn't implement icon support OOTB - that's either something we can look at on EUI's end, or we can try to tweak this plugin in Kibana to use EUI components instead of classes.
There was a problem hiding this comment.
Got it! So for now we are ok, we will revisit when we convert the component to use emotion instead 👍
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Async chunks
Page load bundle
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @1Copenut |
* main: (153 commits)
[Security Solution] {{state.signals_count}} Object not working (#156472) (#156707)
[Synthetics] refresh data on visualization scrubbing (#156777)
[RAM] Docs for slack improvements (#153885)
[RAM] Alert search bar only KQL (#155947)
[ML] Functional tests - stabilize export job tests (#156586)
[Saved Search] Update saved search schema to allow empty `sort` arrays (#156769)
[ML] Rename `curated` model type to `elastic` (#156684)
[Discover] Enable sharing for text based languages (#156652)
[api-docs] 2023-05-05 Daily api_docs build (#156781)
Upgrade EUI to v77.2.2 (#155208)
[RAM][Maintenance Window][8.8]Fix window maintenance workflow (#156427)
[DOCS] Case file attachments (#156459)
[D4C] additional error handling for 'block' action added + policy editor UI fixes (#156629)
[Enterprise Search] refactor(SearchApplications): rename telemetry ids (#156733)
[Enterprise Search] Add telemetry to ELSER deployment buttons + error (#156545)
[Security Solution] fixes Data Quality dashboard errors when a `basePath` is configured (#156233)
[Logs onboarding] StepsFooter outside of main panel (#156686)
[Security Solution] Add a migration to unmute custom Security Solution rules (#156593)
[Enterprise Search][Behavioral Analytics] Update formulas (#156704)
Add API Events to Endpoint Security Advanced Policy (#156718)
...
Summary
[email protected]⏩[email protected]Closes elastic/eui#6724
77.2.2EuiFocusTrapto support thegapModeprop configuration (now defaults topadding) (#6744)Bug fixes
scrollLockproperty onEuiFocusTrap(and other components usingEuiFocusTrap, such asEuiFlyoutandEuiModal) to no longer block scrolling on nested portalled content, such as combobox dropdowns (#6744)77.2.1Bug fixes
EuiFieldNumber's native browser validity detection causing extra unnecessary rerenders (#6741)77.2.0EuiFieldNumberto detect native browser invalid state and show an invalid icon (#6704)EuiRangeandEuiDualRangewhenshowInput={true}to account for invalid icons (#6704)isInvalidstyling ofEuiDualRangewhenshowInput="inputWithPopover"(#6704)EuiFormControlLayoutIconsto render left icons in expected DOM order (#6705)EuiDatePickerRange'sisInvalidstate to match other range inputs (#6705)EuiSuperDatePicker'sisInvalidstate to match other range inputs (#6705)Bug fixes
EuiValidatableControlto correctly displayisInvalidstates on mount (#6705)EuiSearchBarwhere quoted phrases were not quoted when generating an Elasticsearch query. (#6714)