[Maps] EUI Visual Refresh Integration #204434
Conversation
cqliu1
changed the title
cqliu1
changed the title
cqliu1
added
Team:Presentation
|
Pinging @elastic/kibana-presentation (Team:Presentation) |
cqliu1
added
release_note:skip
💔 Build Failed
Failed CI StepsHistory |
| textY: number; | ||
| formattedValue: string | number; | ||
| circleCenterY: number; | ||
| circleStyle: CSSProperties; |
There was a problem hiding this comment.
circleStyle is defined in props but never used in props.
There was a problem hiding this comment.
I am not sure about the direction where EUI themes are only available in a react component. There are times when we need access to EUI themes outside of a react component. For example, to get hex codes to pass to map-libre. I think we need to loop in @elastic/kibana-design and have a discussion about their plans to deprecate euiThemeVars as this will not work for maps and external libraries like maplibre.
| onWidthChange, | ||
| }) => { | ||
| const { euiTheme } = useEuiTheme(); | ||
| const circleStyle = { |
There was a problem hiding this comment.
How about removing circleStyle const here
| value={formattedValue} | ||
| /> | ||
| <circle | ||
| style={{ ...circleStyle, stroke: euiTheme.colors.textParagraph }} |
There was a problem hiding this comment.
Then use circleStyle from props here.
| @@ -62,7 +61,8 @@ export function VectorStyleLegend({ | |||
| ); | |||
| } | |||
|
|
|||
| function renderMasksByFieldOrigin(fieldOrigin: FIELD_ORIGIN) { | |||
| function MasksByFieldOrigin({ fieldOrigin }: { fieldOrigin: FIELD_ORIGIN }) { | |||
There was a problem hiding this comment.
Now that MasksByFieldOrigin is a react component, how about moving it into a separate file?
| @@ -246,9 +246,11 @@ export class DynamicColorProperty extends DynamicStyleProperty<ColorDynamicOptio | |||
| return this._chartsPaletteServiceGetColor('__other__'); | |||
| } | |||
|
|
|||
| const { euiTheme } = useEuiTheme(); | |||
There was a problem hiding this comment.
Looks like euiTheme is unused
| @@ -19,7 +18,7 @@ export const OTHER_CATEGORY_LABEL = i18n.translate( | |||
| } | |||
| ); | |||
|
|
|||
| export const OTHER_CATEGORY_DEFAULT_COLOR = euiThemeVars.euiColorLightShade; | |||
| export const OTHER_CATEGORY_DEFAULT_COLOR_TOKEN = 'textSubdued'; | |||
There was a problem hiding this comment.
'textSubdued' is not going to work. This value is passed to maplibre for rendering and must be a hex value. For example, this is what a map looks like with a value of 'textSubdued'. The reason being is that OTHER_CATEGORY_DEFAULT_COLOR_TOKEN is used to create a maplibre expression. The expression is invalid with a value of 'textSubdued' so maplibre does not include the expression and all styling is lost.
When instead, it should look like this
This should loop in @elastic/eui-team |
Summary
Related to #203132.
Closes #204591.
This replaces all references to euiThemeVars in favor of the useEuiTheme hook in maps.
Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.
release_note:breakinglabel should be applied in these situations.release_note:*label is applied per the guidelinesIdentify risks
Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss.
Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging.