Skip to content

EuiResizableContainer#2701

Merged
thompsongl merged 50 commits intoelastic:masterfrom
sulemanof:resizable_container
Apr 30, 2020
Merged

EuiResizableContainer#2701
thompsongl merged 50 commits intoelastic:masterfrom
sulemanof:resizable_container

Conversation

@sulemanof
Copy link
Contributor

@sulemanof sulemanof commented Dec 19, 2019

Summary

Resolves #1298 .

The Resizable container implementation with two ways of resizing:

  • horizontal

resize_horizontal

  • vertical

resize_vertical

Checklist

  • Check against all themes for compatability in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@sulemanof sulemanof changed the title Resizable container [WIP] Resizable container Dec 21, 2019
@sulemanof
Copy link
Contributor Author

Hey @chandlerprall !
Here is a draft version of the Resizable container.
Sorry, but currently it lacks any documentation 🙄
Could you please take a brief look into it and respond with your feelings ?

@chandlerprall chandlerprall self-requested a review January 9, 2020 16:39
@chandlerprall chandlerprall self-assigned this Jan 9, 2020
@chandlerprall
Copy link
Contributor

Checked out your branch and have focused on the developer experience for consuming/using this component (i.e. resizable_component.js demo and resizable_container.tsx). I really like the direction you took with this and think it can be really strong component. I've listed out some higher-level ideas below and am interested in your thoughts on each.

multiple panels per instance

I think this could be even more useful if a single container can contain more than two panels. I wouldn't mix horizontal + vertical, but multiple panels of the same type across the container would prevent extra boilerplate code from someone building such a layout.

size state

A problem we've run into with other components is developers wanting to 1. reset a component and 2. save the component's state for the next use. Learning from this, one pattern we aimed for with the datagrid is to always pass those state values back up to the consuming application and let it act however it wants. In this case, I'm imagining passing the panel sizes back up to the owner which can store the values however it wants and passes them back into the resizeable container. This also enables an app to reset the state however & whenever it wants, as it owns the state. Further, it removes the concept of initial size.

style as functionality

While I'm not sure I like using CSS to enforce minimum panel sizes, either way (through CSS as it is now or through e.g. Math.max(minimum, currentSize)) I think it'd be better for a developer to have a dedicated prop on Panel to handle this. Panel could then apply the constraint via CSS or JavaScript - in fact if done really well we'd be able to swap between methods in the future with no impact to the owning application.

@flash1293
Copy link
Contributor

@sulemanof Noticed a problem with the resizable panel in the current visualize editor, could you check whether that's also happening here? When the left panel is overflowing, the right panel is sometimes too small (see attached gif)

panl overflow

You can get such a visualization e.g. by creating a table visualization with multiple columns and then splitting the table again with "columns" direction.

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this approach, @sulemanof. Thanks!

Looking through the latest commit, it appears you considered and implemented all of the high-level feedback from @chandlerprall.

I'm going to do a full review, but we'll also have some design feedback coming soon. We're happy to help you get this across the finish line in any way we can, including writing commits against our own feedback if you have priorities elsewhere.

@snide
Copy link
Contributor

snide commented Mar 5, 2020

Also, just an fyi I know that @hbharding is working on a design update for this. Henry, you'll need to merge in those latest changes. Give a yell if you need some help.

Scratch that, no changes since you last pulled. Regardless, might want to push in here before a bunch of review starts happening.

@thompsongl
Copy link
Contributor

might want to push in here before a bunch of review starts happening

I'm happy to wait for @hbharding to do his thing first. Might be a better order anyway

@sulemanof
Copy link
Contributor Author

Hey @thompsongl @chandlerprall
I can't maintain this PR anymore since I'm leaving kibana next week.
I would be happy if this PR was picked by someone and finished once 😃
Hope this draft will be still useful.
If you have any questions, do not hesitate to ping me, I'll answer you in my spare time

@cchaos
Copy link
Contributor

cchaos commented Mar 20, 2020

Thanks for all tour hard work, here and in Kibana! We can easily take over from here. 👋

@thompsongl
Copy link
Contributor

Thanks, @sulemanof 🙌 You've left it in a great spot for us!

@thompsongl thompsongl requested review from thompsongl and removed request for thompsongl March 20, 2020 14:18
@thompsongl
Copy link
Contributor

jenkins test this

@thompsongl
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_2701/

@chandlerprall
Copy link
Contributor

I'm still seeing the issue where resizing with the first handle affects non-adjacent panels

affected panels

@thompsongl
Copy link
Contributor

I'm still seeing the issue where resizing with the first handle affects non-adjacent panels

Fixed by latest commit. Added math to disallow attempting to set a panel size below its specified minSize. The problem you were seeing was a sizing correction by the browser to reset min-width

@thompsongl
Copy link
Contributor

jenkins test this

1 similar comment
@thompsongl
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_2701/

@thompsongl
Copy link
Contributor

jenkins test this

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this looks really neat under the hood! Only had a couple of things stand out.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_2701/

@thompsongl
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_2701/

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM! Pulled and tested locally

@thompsongl thompsongl changed the title Resizable container EuiResizableContainer Apr 30, 2020
@thompsongl thompsongl merged commit 2b9eca6 into elastic:master Apr 30, 2020
@snide snide mentioned this pull request Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Resizer component

9 participants