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

Popover: Expose refresh method from Dropdown component #8906

Merged
merged 1 commit into from Sep 17, 2018

Conversation

Projects
None yet
4 participants
@psealock
Contributor

psealock commented Aug 13, 2018

Description

This PR exposes the refresh method on Popover class and applies a ref to the Popover within the Dropdown component so that refresh method can be accessed.

Why?

When the contents of the Dropdown component change height due to user interaction inside the Dropdown, the Popover element will not re-calculate the height of the content and the scrollable areas are not accessible.

This code allows an app using Dropdown to call the Popover's refresh method to force a recalculation of content height.

How has this been tested?

Rendering an instance of the Dropdown component, I can log to the console this.popoverRef.current and see the class method refresh available.

Corresponding wc-admin issue, woocommerce/wc-admin#252

Screenshots

I'm not sure why some borders look weird in this gif, but you can see the dropdown panel extend beyond the screen after switching to the Custom tab. The PR allows the dropdown to recalculate height and allow the content to be scrollable

scroll

Types of changes

Bug fix (non-breaking change which fixes an issue) : This PR exposes a class method that was already there.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
@timmyc

This comment has been minimized.

Show comment
Hide comment
@timmyc

timmyc Sep 11, 2018

Contributor

@youknowriad is there anyway we could get a review on this PR 👀or is there anything I can do to help get this one moving forward? Thanks!

Contributor

timmyc commented Sep 11, 2018

@youknowriad is there anyway we could get a review on this PR 👀or is there anything I can do to help get this one moving forward? Thanks!

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 11, 2018

Contributor

Can we just add a comment explaining why it's exposed to avoid removing it by accident?

Contributor

youknowriad commented Sep 11, 2018

Can we just add a comment explaining why it's exposed to avoid removing it by accident?

@psealock

This comment has been minimized.

Show comment
Hide comment
@psealock

psealock Sep 13, 2018

Contributor

Thanks for the review @youknowriad !

I've added a comment describing the need for the ref

Contributor

psealock commented Sep 13, 2018

Thanks for the review @youknowriad !

I've added a comment describing the need for the ref

@psealock

This comment has been minimized.

Show comment
Hide comment
@psealock

psealock Sep 13, 2018

Contributor

So if I understand properly, you're going to call dropdown.popoverRef.refresh() right? Would it make sense to add a refresh function to the Dropdown component instead?

Absolutely. Thanks for that @youknowriad . I've added a refresh function to dropdown

Contributor

psealock commented Sep 13, 2018

So if I understand properly, you're going to call dropdown.popoverRef.refresh() right? Would it make sense to add a refresh function to the Dropdown component instead?

Absolutely. Thanks for that @youknowriad . I've added a refresh function to dropdown

@psealock

This comment has been minimized.

Show comment
Hide comment
@psealock

psealock Sep 17, 2018

Contributor

Yikes, good call @youknowriad. I added a check for this.popoverRef.current

Contributor

psealock commented Sep 17, 2018

Yikes, good call @youknowriad. I added a check for this.popoverRef.current

@youknowriad

LGTM 👍

@youknowriad youknowriad merged commit 657db31 into WordPress:master Sep 17, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@psealock psealock deleted the psealock:add/popover-ref-expose-refresh branch Sep 18, 2018

@mtias mtias added the UI Components label Oct 9, 2018

@mtias mtias added this to the 4.0 milestone Oct 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment