[Canvas] Fix: lose element selection on page flip #32111
Conversation
monfera
added
review
v7.0.0
Team:Presentation
|
Pinging @elastic/kibana-canvas |
💚 Build Succeeded |
There was a problem hiding this comment.
The sidebar is still out of sync with the element selection, but now it's inverted. Previously the element would be selected by the sidebar was for the workpad, not the element. Now, nothing is selected, but the sidebar is still for the element that used to be selected.
|
Thanks @w33ble, looking into it |
monfera
force-pushed
the
lose-selection-on-page-flip
branch
from
f91b840
to
6ba1b3d
Compare
💔 Build Failed |
|
retest |
💚 Build Succeeded |
| @@ -320,6 +322,14 @@ export const aeroelastic = ({ dispatch, getState }) => { | |||
| aero.removeStore(action.payload); | |||
| } | |||
|
|
|||
| if (pageChangerActions.indexOf(action.type) >= 0) { | |||
| if (getSelectedElement(getState())) { | |||
There was a problem hiding this comment.
Isn't this the same as prevPage from above (which you're also using in the else condition)?
There was a problem hiding this comment.
prevPage is bound to the result of getSelectedPage, not getSelectedElement to which the comment seems attached.
In any case, the overall point about common subexpression elimination is true. I had a provisional commit, which extracts out all the getState() etc. calls in this file, in some cases, reused like 10 times. I ended up not pushing it because this middleware file is mostly going away if the local (workpadPage) state based approach proves solid, and there is no notable perf difference.
Also, my mental model is slightly hazy if there are middleware statements that dispatch Redux actions: the dispatched action will re-execute the middleware part (I believe synchronously), and coming back from that dispatch to the original middleware call, what you have in getState and what you might have originally bound getState to might deviate. Ideally it's not the case (ie. calling getState from within the same execution of the middleware function should yield the snapshot) but again, I'm lacking a clear model, and I don't currently see how either way can be semantically clear (getState() giving old results, or giving new results both have their problems), so I'd be glad to hear or read info on this.
* Fix: lose element selection on page flip * Fix: remove selection in Redux too (PR feedback from Joe)
* Fix: lose element selection on page flip * Fix: remove selection in Redux too (PR feedback from Joe)
* Fix: lose element selection on page flip * Fix: remove selection in Redux too (PR feedback from Joe)
Fixes #31839 - thanks Joe for the report