Skip to content

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

Merged
thompsongl merged 9 commits intoelastic:masterfrom
shrey:shrey13
Jun 29, 2020
Merged

Sent all the panel sizes via onPanelWidthChange in EuiResizableComponent rather than just the 2 panel sizes#3630
thompsongl merged 9 commits intoelastic:masterfrom
shrey:shrey13

Conversation

@shrey
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

shrey commented Jun 24, 2020

@thompsongl Okay, on it :)

@shrey
Copy link
Copy Markdown
Contributor Author

shrey commented Jun 24, 2020

@thompsongl Done, Is this what was needed?

@shrey
Copy link
Copy Markdown
Contributor Author

shrey commented Jun 24, 2020

@thompsongl size prop would always be present, right?

@thompsongl
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@shrey
Copy link
Copy Markdown
Contributor Author

shrey commented Jun 26, 2020

@thompsongl Done :)

@thompsongl
Copy link
Copy Markdown
Contributor

jenkins test this

@kibanamachine
Copy link
Copy Markdown

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

@shrey
Copy link
Copy Markdown
Contributor Author

shrey commented Jun 26, 2020

@thompsongl tests passed :)

@shrey
Copy link
Copy Markdown
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