Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[ML] Invalid actions pattern on Transforms management table #70383

Closed
thompsongl opened this issue Jun 30, 2020 · 4 comments
Closed

[ML] Invalid actions pattern on Transforms management table #70383

thompsongl opened this issue Jun 30, 2020 · 4 comments
Assignees
@walterra
Labels
bug Fixes for quality problems that affect the customer experience Feature:Transforms ML transforms :ml v7.9.0

Comments

@thompsongl
Copy link
Contributor

thompsongl commented Jun 30, 2020 •
edited
Loading

Upgrading EUI to its latest version (#70243, not yet merged) highlights a component composition pattern that is now incompatible with EUI.

Screen Shot 2020-06-30 at 4 38 44 PM

The TransformsManagement component table row actions (Start, Edit, and Delete) use a pattern where the form component (inside either a modal or a flyout) is rendered from within the actions menu. When the menu closes, the form component is unmounted and closes.
EUI altered this category of table actions menu to close by default after an item action has been taken.

The result is that the Start, Edit, and Delete actions cannot be taken, as the launched components are immediately unmounted.

Any workarounds result in event propagation and focus management issues due to the React portal component tree. Our recommendation is to move the Start, Edit, and Delete form components outside of the menu actions themselves to be renedered by a parent with "higher" state.

EUI could mitigate the problem on a temporary basis by providing an opt-out option for the new behavior. This would require a new release, and the added prop would eventually be deprecated.

Happy to discuss, explain more, or give a demo.

cc// @walterra @peteharverson @alvarezmelissa87
@thompsongl thompsongl added the :ml label Jun 30, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@peteharverson peteharverson changed the title Stack Management | TransformsManagement table and EUI patterns [ML] Invalid actions pattern on Transforms management table Jul 1, 2020
@peteharverson peteharverson added Feature:Transforms ML transforms bug Fixes for quality problems that affect the customer experience v7.9.0 labels Jul 1, 2020
@walterra walterra self-assigned this Jul 1, 2020
@walterra
Copy link
Contributor

walterra commented Jul 1, 2020

Thanks for the ping @thompsongl! We have a related issue here which we wanted to fix for 7.9 because we weren't happy with that code anyway. It needs some refactoring but after discussing with the team we'll pick it up right away. So we'll try to get a fix in before you merge the EUI update so we don't have to disable any tests and you don't have to create another EUI release. Does that sound ok for you?

@thompsongl
Copy link
Contributor Author

That's perfect, @walterra, thanks.
I'll pause the EUI upgrade PR until you've got a fix merged. Ping me on the PR so I can follow along.

@thompsongl thompsongl mentioned this issue Jul 1, 2020
Merged
@walterra walterra mentioned this issue Jul 2, 2020
Merged
5 tasks
walterra added a commit that referenced this issue Jul 8, 2020
@walterra
[ML] Transforms/DFA: Refactor list action buttons so modals won't unm…
bb96f5d 
…ount after button click. (#70555)

Related to #70383 and #63455.

Refactors the action buttons of the transform and data frame analytics jobs list:

Previously custom actions included state and JSX for e.g. confirmation modals. Problem with that: If the actions list popover hides, the modal would unmount too. Since EUI's behaviour will change with the release/merge of #70383, we needed a refactor that solves that issue right now.

With this PR, state management for UI behaviour that follows after a button click like the confirmation modals was moved to a custom hook which is part of the outer level of the buttons itself. The modal now also gets mounted on the outer level. This way we won't loose the modals state and DOM rendering when the action button hides.

Note that this PR doesn't fix the nested buttons issue (#63455) yet. For that we need EUI issue #70383 to be in Kibana which will arrive with EUI v26.3.0 via #70243. So there will be one follow up to that which will focus on getting rid of the nested button structure.
walterra added a commit to walterra/kibana that referenced this issue Jul 8, 2020
@walterra
[ML] Transforms/DFA: Refactor list action buttons so modals won't unm…
af02308 
…ount after button click. (elastic#70555)

Related to elastic#70383 and elastic#63455.

Refactors the action buttons of the transform and data frame analytics jobs list:

Previously custom actions included state and JSX for e.g. confirmation modals. Problem with that: If the actions list popover hides, the modal would unmount too. Since EUI's behaviour will change with the release/merge of elastic#70383, we needed a refactor that solves that issue right now.

With this PR, state management for UI behaviour that follows after a button click like the confirmation modals was moved to a custom hook which is part of the outer level of the buttons itself. The modal now also gets mounted on the outer level. This way we won't loose the modals state and DOM rendering when the action button hides.

Note that this PR doesn't fix the nested buttons issue (elastic#63455) yet. For that we need EUI issue elastic#70383 to be in Kibana which will arrive with EUI v26.3.0 via elastic#70243. So there will be one follow up to that which will focus on getting rid of the nested button structure.
walterra added a commit that referenced this issue Jul 8, 2020
@walterra @elasticmachine
[ML] Transforms/DFA: Refactor list action buttons so modals won't unm…
d4a2c79 
…ount after button click. (#70555) (#71050)

Related to #70383 and #63455.

Refactors the action buttons of the transform and data frame analytics jobs list:

Previously custom actions included state and JSX for e.g. confirmation modals. Problem with that: If the actions list popover hides, the modal would unmount too. Since EUI's behaviour will change with the release/merge of #70383, we needed a refactor that solves that issue right now.

With this PR, state management for UI behaviour that follows after a button click like the confirmation modals was moved to a custom hook which is part of the outer level of the buttons itself. The modal now also gets mounted on the outer level. This way we won't loose the modals state and DOM rendering when the action button hides.

Note that this PR doesn't fix the nested buttons issue (#63455) yet. For that we need EUI issue #70383 to be in Kibana which will arrive with EUI v26.3.0 via #70243. So there will be one follow up to that which will focus on getting rid of the nested button structure.

Co-authored-by: Elastic Machine <[email protected]>
@walterra
Copy link
Contributor

Fixed by #70555.

This was referenced Nov 24, 2020
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@walterra walterra

Labels
bug Fixes for quality problems that affect the customer experience Feature:Transforms ML transforms :ml v7.9.0
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@walterra @thompsongl @peteharverson @elasticmachine