-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Bumping EUI to 71.0.0 #147142
Bumping EUI to 71.0.0 #147142
Changes from all commits
89c139a
996c31a
5a99109
5fffc5c
6f1dfd2
5c575f9
4578f14
107bc37
45e329b
408a33d
0ea2bd9
4c1ef08
50e5962
6c73e40
d950d8e
978c60a
56e45b1
21eaba4
cd832e6
985f451
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -107,7 +107,7 @@ | |
"@elastic/datemath": "5.0.3", | ||
"@elastic/elasticsearch": "npm:@elastic/[email protected]", | ||
"@elastic/ems-client": "8.3.3", | ||
"@elastic/eui": "70.4.0", | ||
"@elastic/eui": "71.0.0", | ||
"@elastic/filesaver": "1.1.2", | ||
"@elastic/node-crypto": "1.2.1", | ||
"@elastic/numeral": "^2.5.1", | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -84,6 +84,6 @@ export const LICENSE_OVERRIDES = { | |
'[email protected]': ['Eclipse Distribution License - v 1.0'], // cf. https://github.com/bjornharrtell/jsts | ||
'@mapbox/[email protected]': ['MIT'], // license in readme https://github.com/tmcw/jsonlint | ||
'@elastic/[email protected]': ['Elastic License 2.0'], | ||
'@elastic/eui@70.4.0': ['SSPL-1.0 OR Elastic License 2.0'], | ||
'@elastic/eui@71.0.0': ['SSPL-1.0 OR Elastic License 2.0'], | ||
'[email protected]': ['CC-BY-4.0'], // retired ODC‑By license https://github.com/mattcg/language-subtag-registry | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ | |
* Side Public License, v 1. | ||
*/ | ||
|
||
import React, { FC, useEffect, useRef, useState } from 'react'; | ||
import React, { FC, ComponentProps, Ref, useEffect, useState } from 'react'; | ||
import useMount from 'react-use/lib/useMount'; | ||
|
||
import { | ||
|
@@ -18,6 +18,7 @@ import { | |
EuiToolTip, | ||
EuiButtonIcon, | ||
} from '@elastic/eui'; | ||
import type { EuiDualRangeClass } from '@elastic/eui/src/components/form/range/dual_range'; | ||
import { useReduxEmbeddableContext } from '@kbn/presentation-util-plugin/public'; | ||
|
||
import { RangeValue } from '../../../common/range_slider/types'; | ||
|
@@ -26,9 +27,11 @@ import { rangeSliderReducers } from '../range_slider_reducers'; | |
import { RangeSliderReduxState } from '../types'; | ||
import { RangeSliderStrings } from './range_slider_strings'; | ||
|
||
export const RangeSliderPopover: FC = () => { | ||
// Unfortunately, wrapping EuiDualRange in `withEuiTheme` has created this annoying/verbose typing | ||
export type EuiDualRangeRef = EuiDualRangeClass & ComponentProps<typeof EuiDualRange>; | ||
|
||
export const RangeSliderPopover: FC<{ rangeRef?: Ref<EuiDualRangeRef> }> = ({ rangeRef }) => { | ||
const [fieldFormatter, setFieldFormatter] = useState(() => (toFormat: string) => toFormat); | ||
const rangeRef = useRef<EuiDualRange | null>(null); | ||
|
||
// Controls Services Context | ||
const { | ||
|
@@ -143,8 +146,8 @@ export const RangeSliderPopover: FC = () => { | |
<EuiFlexItem> | ||
<EuiDualRange | ||
id={id} | ||
min={hasAvailableRange ? rangeSliderMin : undefined} | ||
max={hasAvailableRange ? rangeSliderMax : undefined} | ||
min={hasAvailableRange ? rangeSliderMin : 0} | ||
max={hasAvailableRange ? rangeSliderMax : 100} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tested this by getting the range slider control into a no-data state, and it looks the same as it did before. 👌 |
||
onChange={([newLowerBound, newUpperBound]) => { | ||
const updatedLowerBound = | ||
typeof newLowerBound === 'number' ? String(newLowerBound) : value[0]; | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Is there anything we can do on our side to get rid of these complex types? Maybe a forward ref? Or some other way of getting a handle on the class?
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.
Honestly? Without knowing exactly why you're calling the internal
EuiDualRange
's internalonResize
class method... my recommendation is to not do that 😅 (which removes the need for theref
entirely, and thus the ref typing shenanigans).If we ever convert
EuiRange
/EuiDualRange
to functional non-class components as a tech debt item, you'd lose access to this API entirely (it's not something we're deliberately exposing or documenting as a consumer API). I did create a regression test to ensure we continued exposingonResize
via ref, just for this specific Kibana usage of it, but honestly, we'd prefer y'all not reach for it at all, or figure out what problem it's solving and have us try to solve it in a less technically hacky way.I don't have the context as to why that original call was added or what it's doing - but it's definitely a code smell that makes me think either this usage of it is trying to do things EuiDualRange is not meant to do, or is super custom and undoing things EuiDualRange does and then re-doing them later (also not great).
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.
If I remember correctly, this usage is due to us not being able to use
EuiInputPopover
, but still wanting to have the popover to be tied to the width of the input. I can't exactly remember why we couldn't useEuiInputPopover
though.I wonder if we would run into any regressions by removing that call entirely? @cqliu1, do you remember why we had to use
EuiPopover
?