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

Popover: consistently adjust position on scroll #17867

Merged
merged 4 commits into from Nov 27, 2019
Merged

Conversation

@ellatrix
Copy link
Member

ellatrix commented Oct 9, 2019

Description

Currently some popovers won't reposition when the page is scrolled. These include: the link popover, image, and autocomplete.

The problem is that the rectangle is NOT calculated by Popover, but by the component implementing it. In other word, the anchorRect is no good if you want the popover to reposition on scroll. I'm not sure what the prop would be good for, so perhaps we should look into deprecating it if no core components use it.

getAnchorRect would be an option, but I believe we can provide a better API than that. Some implementation might want to adjust the rectangle: add a buffer or include the padding of the target anchor. It seems much cleaner for the implementor to just pass the anchor reference, and let Popover compute the anchor rect and adjust it when needed.

So I added a few more props for the Popover component. The old ones will still continue to work.

  • anchorRef: a reference to the anchor, which can be either an Element or a Range (the browser can give us a DOMRect for both of these).
  • anchorIncludePadding: can be used if you want to include the anchor padding when creating a rectangle.
  • anchorVerticalBuffer and anchorHorizontalBuffer can be used to add a buffer/offset between the anchor and the popover.

How has this been tested?

Open the link, image, or slash command popover. Scroll the page.

Screenshots

popover-scroll

Types of changes

Bug fix.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
anchorRef,
anchorWithoutPadding,
anchorVerticalBuffer,
anchorHorizontalBuffer,

This comment has been minimized.

Copy link
@youknowriad

youknowriad Oct 10, 2019

Contributor

Do you think we should have a single prop instead with different options? anchor = { ref, padding, buffer }

This comment has been minimized.

Copy link
@ellatrix

ellatrix Oct 10, 2019

Author Member

I don't like nesting that much for component props. That would break shallow comparison, right? Maybe we can name them differently...

This comment has been minimized.

Copy link
@youknowriad

youknowriad Oct 10, 2019

Contributor

It does break shallow comparison. we can use things like useMemo to avoid that but I'm not sure if it's worth it.

I like the approach of this PR, maybe we can get more thoughts on props naming here @mcsf

This comment has been minimized.

Copy link
@youknowriad

youknowriad Oct 10, 2019

Contributor

I wonder how other popover libraries or design systems name/handle the positioning options. cc @ItsJonQ

This comment has been minimized.

Copy link
@ItsJonQ

ItsJonQ Oct 10, 2019

Contributor

@youknowriad + @ellatrix 👋 Halloo!

A popular popover/positioner library folks use (under the hood) is Popper.js

Their React implementation uses the main library and share the same API.
They use nested objects, example:

offsets.popper
top, left, width, height values

data.offsets.reference
top, left, width, height values

I've used both shallow and nested approaches before. Both have their pros and cons :).

Typically component library components/primitives have lots of props. Way more compared to components created at the app level.

If this were going into@wordpress/components to update the Popover, I would vote for @youknowriad 's suggestion with nested.

However, since this is updating an "application" component, something that typically won't be consumed externally and used to build interfaces... I would agree with you @ellatrix, and vote for shallow.

Hope this helps! <3

This comment has been minimized.

Copy link
@ellatrix

ellatrix Oct 10, 2019

Author Member

I've used both shallow and nested approaches before. Both have their pros and cons :).

Could you elaborate? What are the pros of nesting? I'm glad to see that Popper also requires an element reference, just as I'm proposing here.

This comment has been minimized.

Copy link
@ItsJonQ

ItsJonQ Oct 10, 2019

Contributor

@ellatrix

I'm glad to see that Popper also requires an element reference, just as I'm proposing here.

❤️

A pro of nesting would be that it keeps related props grouped together.
Also makes them easier to work with (IMO) those props are passed within a render prop.

I would only nest 1 level deep (2 max!!! if you really really need to)

An example would be react-select. The components prop is a good one. It allows the user to pass in custom components to replace their sub-components (e.g. Menu). If that wasn't there, they might need to define something like componentMenu, componentInput, etc...

It's not the end of the world. But that library has a ton of props. I think this keeps it tidier :)

That being said.. if one decides to nest, they'll need to do things like (as @youknowriad mentioned) memoize. Also to do some sort of defaultValues + userDefinedValues merging.

This comment has been minimized.

Copy link
@ellatrix

ellatrix Oct 10, 2019

Author Member

useMemo puts extra responsibilities on the implementor. Not sure if it's worth it for these few props.

This comment has been minimized.

Copy link
@ItsJonQ

ItsJonQ Oct 10, 2019

Contributor

🤦‍♂ My apologies! I didn't realize these changes were on @wordpress/components/popover.
(It was early for me)

useMemo puts extra responsibilities on the implementor.

I agree. These primitives should take care of this overhead.

If we were to use an object, would exporting it like this help?

import { useRef, useState, useEffect, memo } from '@wordpress/element';
...
const PopoverContainer = memo(Popover);

I'm guessing not, since memo only does shallow comparisons.

Not sure if it's worth it for these few props.

Given its current state, I think it might be better to keep it simple (and shallow).
My concern was maybe later the number of props + names of them may get unwieldy, but that may not happen :).

Apologies for the back and forth!

This comment has been minimized.

Copy link
@mcsf

mcsf Nov 27, 2019

Contributor

I mean, the interface provider can always manage itself with shouldComponentUpdate, and I get the benefit of keeping things tidy with nested props.

The question is probably how much we expect these interfaces to grow, since we aren't building an all-encompassing popover library. My first impression is that it's probably not worth the extra complexity to have nested props.

I like the approach of this PR, maybe we can get more thoughts on props naming here @mcsf

I think this PR is good. I would just double-check that the props are named in a way that conveys their type well, as pointed out in https://github.com/WordPress/gutenberg/pull/17867/files#r351238110.

if ( ! anchorRef.current ) {
return;
}
let newAnchor = computeAnchorRect(

This comment has been minimized.

Copy link
@youknowriad

youknowriad Oct 10, 2019

Contributor

I don't see much difference betweencomputeAnchorRect and useAnchor. I mean we could just inline it here. What's the reasoning?

This comment has been minimized.

Copy link
@ellatrix

ellatrix Oct 10, 2019

Author Member

It's clearer to me to make early returns than to wrap inside else blocks. :)

This comment has been minimized.

Copy link
@ellatrix

ellatrix Oct 10, 2019

Author Member

And to return a value to a constant instead of assigning it to a variable.

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Nov 27, 2019

@youknowriad @ItsJonQ Anything left here?

@ellatrix ellatrix force-pushed the try/popover-parameters branch from dda3afd to 3af3643 Nov 27, 2019
* @param {Object} anchorRect Anchor Rect prop used to override the computed value.
* @param {Function} getAnchorRect Function used to override the anchor value computation algorithm.
* @param {Object} anchorRef Reference to the popover anchor fallback element.
* @param {Object} anchorIncludePadding Whether to include the anchor padding.

This comment has been minimized.

Copy link
@mcsf

mcsf Nov 27, 2019

Contributor

Type should be boolean.

This comment has been minimized.

Copy link
@mcsf

mcsf Nov 27, 2019

Contributor

The buffer props below should have their type corrected too.

Similarly, anchorRef should be better defined, as it seems to assume many shapes: Range, Boolean, maybe also WPElement? Do we need a new type definition for it, and is the "ref" portion of the name still apt?

Finally, I recommend that anchorIncludePadding be rephrased so as to convey that it's a Boolean, with a should prefix.

This comment has been minimized.

Copy link
@ellatrix

ellatrix Nov 27, 2019

Author Member

I updated the types. anchorRef can be an Element or a Range. I think the naming is fine as there are references to a live element or range.

I changed anchorIncludePadding to shouldAnchorIncludePadding, but I find the naming a bit strange. Maybe includeAnchorPadding or shouldIncludeAnchorPadding?

This comment has been minimized.

Copy link
@mcsf

mcsf Nov 27, 2019

Contributor

I think all names will be strange :P. This is when I miss Scheme's or Ruby's use of question marks for predicates (e.g. (empty? x) or x.empty?) — not that this helps in the conversation.

There's also anchorShouldIncludePadding? Honestly, as long as should or a comparable modal is there, I'm happy with whatever. :)

ellatrix added 2 commits Nov 27, 2019
@ellatrix ellatrix requested a review from mcsf Nov 27, 2019
Copy link
Contributor

mcsf left a comment

This is looking quite good. From the code I'm only wondering about performance in useAnchor; specifically, I'm looking at refreshAnchorRect, whether it should be memoised, and also at the getAnchorRect prop — which, since it's a function, places the burden of reusing function references on the interface user.

But the only thing that really matters is overall performance impact. Our perf suite is focused entirely on typing, but it's all we have for now. Have you compared perf results against master for this branch?

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Nov 27, 2019

@mcsf I'm not sure how the performance test would help as there are no popovers rendering during the test. I'll memoize refreshAnchorRect.

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Nov 27, 2019

@mcsf Hm, we shouldn't memoize that function or anything in it because it calculates a new position based on the bounding rectangles. So if you scroll, we need a new rect for the same element or range with the same options.

@mcsf

This comment has been minimized.

Copy link
Contributor

mcsf commented Nov 27, 2019

I'm not sure how the performance test would help as there are no popovers rendering during the test

There should be no difference in perf, but if there were that would be a smell to address. :)

we shouldn't memoize that function or anything in it because it calculates a new position based on the bounding rectangles

👍

@mcsf
mcsf approved these changes Nov 27, 2019
@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Nov 27, 2019

Looks like it's the opposite. Master is a tiny bit slower.

Master:
Average time to load: 4497ms
Average time to DOM content load: 4215ms
Average time to type character: 70.35ms
Slowest time to type character: 128ms
Fastest time to type character: 58ms

This branch:
Average time to load: 4555ms
Average time to DOM content load: 4275ms
Average time to type character: 68.79ms
Slowest time to type character: 130ms
Fastest time to type character: 55ms

@ellatrix ellatrix merged commit 336f183 into master Nov 27, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@youknowriad youknowriad deleted the try/popover-parameters branch Nov 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.