-
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
De-angularize visLegend #50613
De-angularize visLegend #50613
Conversation
💔 Build Failed |
💔 Build Failed |
@nickofthyme I merged upstream into your branch, just FYI. |
aria-selected={color === getColor(legendItem.label)} | ||
onClick={setColor(legendItem.label, color)} | ||
onKeyPress={setColor(legendItem.label, color)} | ||
className={classNames([ |
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.
nit: You don't need the array, classNames also works on the arguments array
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.
Good to know! c002d9f
|
||
export const VisLegend = memo( | ||
VisLegendComponent, | ||
(prev, next) => prev.refreshLegend === next.refreshLegend |
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.
This looks dangerous - I'm not sure whether it can happen, but what if the control flow is changed in the future and the vis object changes without refreshLegend being bumped? Is there something that prevents us from using the default implementation of memo
?
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.
deleted in efb60da
|
||
useEffect(() => { | ||
if (visData) { | ||
refresh(); |
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.
Do we need this refresh-hack in react? IIRC the refreshLegend
thing is there to re-render the legend because vis
and uiState
are mutable objects that are not replaced but just mutated. But it seems like having refreshLegend
as a prop on the react component will make sure the component is re-rendered every time refreshLegend
is changed, so we don't need a separate refresh function but can retrieve the labels and table aggs directly in the render function (without state and refresh hook). I'm always in favor of keeping state out of the components and it seems like this is possible here.
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.
Now only calling refresh
in componentDidMount
see comment below.
} | ||
|
||
if (vislibVis.visConfig) { | ||
getColor = vislibVis.visConfig.data.getColorFunc(); |
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.
this shouldn't go into a module-scoped variable because it can cause errors if multiple visualizations with different vis objects are active at the same time. It seems like we can use a local variable here.
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.
Good point, I pulled it inside class component see efb60da
}; | ||
|
||
export const VisLegendItem = memo(VisLegendItemComponent, (prev, next) => { | ||
return ( |
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 a specific reason for the custom equality check? IMHO these are dangerous because a change in the logic can cause an error in the future which is hard to track down. If it's just to save a few CPU cycles, we should err on the side of calculating the items too often instead of too seldom. 👍 for using memo though, that's a quick and easy optimization.
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.
Yeah I was just testing the render cycles. I didn't know the memo automatically does a shallow compare.
removed in efb60da
const bwcAddLegend = vis.params.addLegend; | ||
const bwcLegendStateDefault = bwcAddLegend == null ? true : bwcAddLegend; | ||
const newOpen = !uiState.get('vis.legendOpen', bwcLegendStateDefault); | ||
setOpen(newOpen); |
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.
Can you check whether anything bad happens in we don't even hold the open state locally in the component but just use vis.legendOpen
directly? I know it's not exactly like the old behavior but this is probably in place because of some weird angular behavior and I would be glad to get rid of it to be able to don't have unnecessary local state in the component.
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.
I'll check it out.
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.
looks to work fine c913d98
💔 Build Failed |
@timroes & @flash1293 I got it to work using the From what I can tell the component is basically acting like a separate App once mounted, essentially cutting off any external prop updates. This requires the unmount and remount of the legend component. I tried calling I don't know if there are any implications from effectively cutting off the legend prop changes, I hope the functional tests will help with that. The nested prop mutations will still be updated but might not ever trigger a rerender, unless forced. If you guys know of a better way to do this lmk, I'm open to ideas 🤔. |
💔 Build Failed |
💔 Build Failed |
💚 Build Succeeded |
💔 Build Failed |
💚 Build Succeeded |
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.
Moving back to standard <button>
for the legend toggle is fine for now. I just had one comment about inconsistent focus states. But when that's done, it will LGTM from a UI/EUI perspective.
<button | ||
type="button" | ||
onClick={this.toggleLegend} | ||
className={classNames('visLegend__toggle', { |
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.
Just one extra thing to match that of the overflow menu button. Can you add this class
className={classNames('visLegend__toggle', { | |
className={classNames('visLegend__toggle kbn-resetFocusState', { |
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.
67d3172
💔 Build Failed |
💔 Build Failed |
@elasticmachine merge upstream |
💚 Build Succeeded |
…ra/kibana into IS-46410_remove-@kbn/ui-framework * 'IS-46410_remove-@kbn/ui-framework' of github.com:mbondyra/kibana: (49 commits) [ML] Re-activate after method in transform test (elastic#51815) [SIEM] [Detection Engine] Add edit on rule creation (elastic#51670) De-angularize visLegend (elastic#50613) [SIEM][Detection Engine] Change security model to use SIEM permissions [Monitoring] Sass cleanup (elastic#51100) Move errors and validate index pattern ⇒ NP (elastic#51805) fixes pagination tests (elastic#51822) Split legacy plugin discovery, expose SavedObjects scopedClient, wrappers, repository (elastic#48882) [SIEM][Detection Engine] Adds ecs threat properties to rules (elastic#51782) [Lens] Remove client-side reference to server source code (elastic#51763) Fix infinite redirect loop when multiple cookies are sent (elastic#50452) fixes drag and drop in tests (elastic#51806) [Console] Proxy fallback (elastic#50185) Query String(Bar) Input - cleanup (elastic#51598) shim visualizations plugin (elastic#50624) Expressions service fixes: better error and loading states handling (elastic#51183) fixes url state tests (elastic#51746) fixes browser field tests (elastic#51738) [ML] Fix anomaly detection test suite (elastic#51712) [SIEM] Fix Timeline drag and drop behavior (elastic#51558) ...
…license-management * 'master' of github.com:elastic/kibana: (48 commits) Enable alerting and actions plugin by default (elastic#51254) Fix error returned when creating an alert with ES security disabled (elastic#51639) [Discover] Improve Percy functional tests (elastic#51699) fixes timeline data providers tests (elastic#51862) [Dependencies]: upgrade react to latest v16.12.0 (elastic#51145) Allow routes to define some payload config values (elastic#50783) Move saved queries service + language switcher ⇒ NP (elastic#51812) [ML] Re-activate after method in transform test (elastic#51815) [SIEM] [Detection Engine] Add edit on rule creation (elastic#51670) De-angularize visLegend (elastic#50613) [SIEM][Detection Engine] Change security model to use SIEM permissions [Monitoring] Sass cleanup (elastic#51100) Move errors and validate index pattern ⇒ NP (elastic#51805) fixes pagination tests (elastic#51822) Split legacy plugin discovery, expose SavedObjects scopedClient, wrappers, repository (elastic#48882) [SIEM][Detection Engine] Adds ecs threat properties to rules (elastic#51782) [Lens] Remove client-side reference to server source code (elastic#51763) Fix infinite redirect loop when multiple cookies are sent (elastic#50452) fixes drag and drop in tests (elastic#51806) [Console] Proxy fallback (elastic#50185) ...
* deangularize visLegend * update vislib controller to mount react legend directly * convert legend components to eui * Position popover based on legend position * Styles cleanup including removing of unused/unnecessary styles
* deangularize visLegend * update vislib controller to mount react legend directly * convert legend components to eui * Position popover based on legend position * Styles cleanup including removing of unused/unnecessary styles
* deangularize visLegend * update vislib controller to mount react legend directly * convert legend components to eui * Position popover based on legend position * Styles cleanup including removing of unused/unnecessary styles
* De-angularize visLegend (#50613) * deangularize visLegend * update vislib controller to mount react legend directly * convert legend components to eui * Position popover based on legend position * Styles cleanup including removing of unused/unnecessary styles * Fix type error with jest tests (#51925)
* deangularize visLegend * update vislib controller to mount react legend directly * convert legend components to eui * Position popover based on legend position * Styles cleanup including removing of unused/unnecessary styles
Summary
De-angularize
visLegend
component.vislib-legend
directive withreactDirective
vislibVisController
to pass props/attributes to new legendUpdate components with EUI
Checklist
For maintainers