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.

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

Sent all the panel sizes via onPanelWidthChange in EuiResizableComponent rather than just the 2 panel sizes #3630

Merged
merged 9 commits into from
Jun 29, 2020

Conversation

shrey
Copy link
Contributor

@shrey shrey commented Jun 17, 2020

Summary

Sending all the panel sizes via the onPanelWidthChange function in EuiResizableComponent rather than just the size for the 2 panels.

Checklist

@kibanamachine
Copy link

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?

@chandlerprall chandlerprall self-requested a review June 18, 2020 16:49
@chandlerprall
Copy link
Contributor

but there's some issue with the value, what do you suggest?

You're adding the size of the panels as pixels, where the existing onPanelWidthChange callback specifies the values in percentages.

Also,

  • When adding onPanelWidthChange={console.log} to the three panel example, the panels should continue to be resizable. This change between controlled/uncontrolled should be determined by the existence of the size prop, but is currently solely based on onPanelWidthChange (https://github.com/elastic/eui/pull/3630/files#diff-021245ddce286351697a251b107f2c72R150)
  • there are a few lint errors, please run yarn test locally to identity them and clean them up. There is a commit hook in place that is supposed to validate changes before they can be committed; I'm curious what you're using to commit your changes, that doesn't seem to be running the hook? (git cli, tower, etc)

@shrey
Copy link
Contributor Author

shrey commented Jun 19, 2020

@chandlerprall Yeah, I forgot to fix the lint errors, sorry, I used git commit -m "" -n as the usual commit used to take too much time

if (prevPanelSize!==nextPanelSize && onPanelWidthChange), will this suffice?

@shrey
Copy link
Contributor Author

shrey commented Jun 23, 2020

@chandlerprall I've fixed the lint error and the percentages, could you review it

@thompsongl thompsongl self-requested a review June 23, 2020 22:15
@thompsongl
Copy link
Contributor

This point still needs to be addressed

When adding onPanelWidthChange={console.log} to the three panel example, the panels should continue to be resizable. This change between controlled/uncontrolled should be determined by the existence of the size prop, but is currently solely based on onPanelWidthChange (https://github.com/elastic/eui/pull/3630/files#diff-021245ddce286351697a251b107f2c72R150)

When setting onPanelWidthChange and not setting size on the panels, no resize changes actually occur.

@shrey
Copy link
Contributor Author

shrey commented Jun 24, 2020

@thompsongl Yeah, I did notice that, but I think when user uses the onPanelWidthChange property, the size change is orchestrated by the user itself in the examples, because I even checked in the original docs example, if I solely have a onPanelWidthChange property and console.log the sizes, the draggable panel continues to be non-responsive

@thompsongl
Copy link
Contributor

I think when user uses the onPanelWidthChange property, the size change is orchestrated by the user itself

This is how the component currently works, but the goal of the issue is to change that behavior. When onPanelWidthChange is in use and the consumer is not orchestrating the panel sizes with the size prop, the panels should resize as though onPanelWidthChange is not set. The size prop should be used to determine controlled vs. uncontrolled behavior, not the onPanelWidthChange prop.

The use case is that a consumer might want to perform a side effect with the panel width values (provided by the onPanelWidthChange callback), but have the component automatically adjust sizes while dragging.

@shrey
Copy link
Contributor Author

shrey commented Jun 24, 2020

@thompsongl Okay, on it :)

@shrey
Copy link
Contributor Author

shrey commented Jun 24, 2020

@thompsongl Done, Is this what was needed?

@shrey
Copy link
Contributor Author

shrey commented Jun 24, 2020

@thompsongl size prop would always be present, right?

@thompsongl
Copy link
Contributor

size prop would always be present, right?

No, the initialSize prop can be used instead for uncontrolled components. Looks like the logic was already in place for size to guard against unnecessary state updates.

@shrey
Copy link
Contributor Author

shrey commented Jun 25, 2020

size prop would always be present, right?

No, the initialSize prop can be used instead for uncontrolled components. Looks like the logic was already in place for size to guard against unnecessary state updates.

Yeah, So Do I need to make any changes now?

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.

About ready! Still needs a changelog entry

src/components/resizable_container/helpers.ts Outdated Show resolved Hide resolved
src/components/resizable_container/helpers.ts Outdated Show resolved Hide resolved
@shrey
Copy link
Contributor Author

shrey commented Jun 26, 2020

@thompsongl Done :)

@thompsongl
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

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

@shrey
Copy link
Contributor Author

shrey commented Jun 26, 2020

@thompsongl tests passed :)

@shrey
Copy link
Contributor Author

shrey commented Jun 28, 2020

@thompsongl any updates?

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.

4 participants