Move SplitPanel to kibana_react#55529
Conversation
jloleysens
added
chore
v8.0.0
release_note:skip
clintandrewhall
left a comment
clintandrewhall
left a comment
There was a problem hiding this comment.
Thanks for moving this upward, and so quickly! I may contribute some changes in the coming days... I noticed there's an opportunity to wrap children in Panels, rather than forcing the consumer to wrap each panel content, for example.
|
FYI there is already a draft PR for moving this to EUI - elastic/eui#2701 @jloleysens if you are moving forward with that, let @sulemanof know. |
|
@flash1293 Thanks for referencing that work here! I wasn't aware those changes were happening I just noticed that this component was already being used in another app after #49864 was merged (I wasn't aware of this either until recently). @clintandrewhall also asked whether he could use it so I moved this out of Console. I guess it is not worth making changes to this component here in future but we can still go ahead with the move for now? |
|
In other words, moving the component shouldn't affect their work other than it not being really necessary as it will probably be removed from Kibana at some future point anyway 😅. |
|
@jloleysens We discussed moving it into kibana_react as part of the PR you linked, but decided against it because the EUI component should be close. With other consumers it definitely makes sense, so thanks for working on this! |
| [onPanelWidthChange] | ||
| ); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Maybe wrap this in
if (process.env.NODE_ENV !== 'production') {There was a problem hiding this comment.
thirdChild could be null or undefined or "" (empty string), maybe
| }, [thirdChild ? 1 : 0]); | |
| }, [children.length > 2]); |
++ Yeah this is definitely the type of thing that belongs in EUI. If elastic/eui#2701 is going to be on the way soon, then ideally we would remove this component as soon as that replacement one becomes available, so that we aren't maintaining it any longer than necessary. Perhaps we could even add a @clintandrewhall If you are looking to contribute changes, maybe you could instead coordinate with @sulemanof to make sure the EUI component has the features you need? That way we aren't deepening our integration with this component. |
|
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* split_panel component -> kibana_react * Update useEffect for console warning * `console` -> `kibana-react` i18n namespace * Update when warning about children is emitted in split panel component Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
* split_panel component -> kibana_react * Update useEffect for console warning * `console` -> `kibana-react` i18n namespace * Update when warning about children is emitted in split panel component Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
7 participants
Summary
Moves the SplitPanel component to a shared location rather than housing it inside of the legacy
Consoleplugin where it can be used and updated.This component creates a divider between two panels that supports dragging the divider which resizes components - currently supports two panels. No reason we can't support n panels if there is a need.
This might be of interest to design to take over eventually.