[Popover] Add preferInputActivator prop to Popover #2754

attila-berki · 2020-02-18T21:16:13Z

Co-authored-by: Robin Drexler robin.drexler@shopify.com

WHY are these changes introduced?

When the activator of a Popover (or PositonedOverlay) contains an input element, the Overlay always uses said element to calculate its position and width/height.

WHAT is this pull request doing?

Introduces a preferInputActivator prop to the Popover (and hence PositonedOverlay component). This prop would be true by default to not introduce a breaking change.

When it's true, the behavior does not change. The Overlay will continue to search for an input, and if it finds one, use it. If the consumer sets the prop to false, the Overlay will never search for an input and always use the activator to calculate its dimensions.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

If you set the preferActivatorInput to false the Popover should expand to the size of the Textfield. By removing the property it should only have the width of the nested input node.

Before:

After:

Copy-paste this code in playground/Playground.tsx:

import React from 'react';
import {SearchMinor} from '@shopify/polaris-icons';
import {Page, Popover, TextField, Checkbox, Icon} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      <Popover
        active
        onClose={() => {}}
        fullWidth
        preferredAlignment="left"
        preferInputActivator={false}
        activatorWrapper="span"
        activator={
          <div>
            <TextField
              label="TextField"
              onChange={() => {}}
              prefix={<Icon source={SearchMinor} color="inkLightest" />}
            />
          </div>
        }
      >
        <div>
          <Checkbox label="Salut" checked={false} onChange={() => {}} />
        </div>
      </Popover>
    </Page>
  );
}

🎩 checklist

Tested on mobile
Tested on multiple browsers
Tested for accessibility
Updated the component's README.md with documentation changes
Tophatted documentation changes in the style guide
For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

robin-drexler · 2020-02-18T21:17:56Z

src/components/PositionedOverlay/tests/PositionedOverlay.test.tsx

+      );
+
+      expect(
+        getRectForNodeSpy.mock.calls.some(([node]) => node === input),


Unfortunately, we can't check for the correct Rect being passed in the render prop because jsdom will always return 0 for all values. So we thought this is the second best way of assuring that the input has been used.

Also getRectForNode is called multiple times for different elements so we could not just use toHaveBeenCalledWith. We didn't want to use toHaveBeenNthCalledWith because this would make the test less resilient to a slightly changing implementation.

Makes sense. Maybe @AndrewMusgrave will have opinion about this.

github-actions · 2020-02-18T21:18:13Z

🟡 This pull request modifies 8 files and might impact 61 other files. This is an average splash zone for a change, remember to tophat areas that could be affected.

Details:

All files potentially affected (total: 61)

📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Card/tests/Card.test.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Popover/Popover.tsx (total: 56)

Files potentially affected (total: 56)

🧩 src/components/Popover/components/PopoverOverlay/PopoverOverlay.tsx (total: 57)

Files potentially affected (total: 57)

🧩 src/components/Popover/components/PopoverOverlay/tests/PopoverOverlay.test.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Popover/tests/Popover.test.tsx (total: 0)

Files potentially affected (total: 0)

🧩 src/components/PositionedOverlay/PositionedOverlay.tsx (total: 61)

Files potentially affected (total: 61)

🧩 src/components/PositionedOverlay/tests/PositionedOverlay.test.tsx (total: 0)

Files potentially affected (total: 0)

dleroux · 2020-02-19T13:20:28Z

src/components/Popover/tests/Popover.test.tsx

  });

+  it("passes 'preferInputActivator' to PopoverOverlay", () => {
+    const popover = mountWithAppProvider(


small nit, we're trying to switch over the react-testing as much as possible. If you can, change this to use mountWithApp instead and avoid the testId.

We actually thought about that, but wanted to keep the test file consistent and not use different testing libraries in one file.
Depending on the effort, we'd try to refactor the entire file.

That would be great if you could. We have been mixing though :|

We updated the entire test file, could you please take a look?

That's awesome thank you! 1 test is failing and there's a merge conflict but apart from that, it looks good!

dleroux

Made a few comments. Let me know what you think.

dleroux · 2020-02-19T13:26:33Z

src/components/PositionedOverlay/PositionedOverlay.tsx

+          preferInputActivator = true,
        } = this.props;

        const textFieldActivator = activator.querySelector('input');


Suggested change

const textFieldActivator = activator.querySelector('input');

const activator = preferInputActivator ? activator.querySelector('input') : activatorProp

Above where you deconstruct the props, I woud just rename activator to activator: activatorProp

Wouldn't that introduce a breaking change? In its current state activatorRect will always be based on an existing node. If the input does not exist, we use the activator

With your suggested change, we might end up with null. When preferInputActivator is true, but there's no input.

Since preferInputActivator defaults to true this would probably break most of the usages where no TextField is the activator

yes my bad. I missed the part we do check for null today. Was trying to get away for querying the for the input if preferInputActivator is false.

dleroux · 2020-02-19T13:32:55Z

src/components/PositionedOverlay/PositionedOverlay.tsx


        const textFieldActivator = activator.querySelector('input');

        const activatorRect =


Suggested change

const activatorRect =

const activatorRect = getRectForNode(activator);

The reason I am making these suggestions is that we won't do an unnecessary activator.querySelector('input') if preferInputActivator is false, but also because by letting people specify preferInputActivator and then doing this logic, the component might still work but not return what was intended. Does that make sense?

dleroux · 2020-02-19T13:34:49Z

src/components/PositionedOverlay/tests/PositionedOverlay.test.tsx

+      );
+
+      expect(
+        getRectForNodeSpy.mock.calls.some(([node]) => node === input),


Makes sense. Maybe @AndrewMusgrave will have opinion about this.

robin-drexler · 2020-02-19T20:50:34Z

src/components/PositionedOverlay/PositionedOverlay.tsx


-        const textFieldActivator = activator.querySelector('input');
+        const preferredActivator = preferInputActivator
+          ? activator.querySelector('input') || activator


We now only query for input when preferredActivator is true.

robin-drexler · 2020-02-20T02:56:23Z

We'll update the conflicting UNRELEASED.md as soon as this PR is approved and we intend to ship. The file unfortunately conflicts pretty quickly and often. 😅

src/components/Popover/Popover.tsx

dleroux · 2020-02-20T13:12:42Z

UNRELEASED.md

 - Fixed focus state color on monochrome `Buttons` ([#2684](https://github.com/Shopify/polaris-react/pull/2684))
 - Fixed container's width on `Modal` ([#2692](https://github.com/Shopify/polaris-react/pull/2692))
 - Fixed the position property for the backdrop on `Select` from being overwritten by the focus ring ([#2748](https://github.com/Shopify/polaris-react/pull/2748))
+- Fixed the `Popover` reference position calculation ([#2752](https://github.com/Shopify/polaris-react/issues/2752)) ([#1415](https://github.com/Shopify/polaris-react/issues/1415))


Suggested change

- Fixed the `Popover` reference position calculation ([#2752](https://github.com/Shopify/polaris-react/issues/2752)) ([#1415](https://github.com/Shopify/polaris-react/issues/1415))

This should probably go into the "Enhancements" section. Also, the link simply points to the PR as opposed to the issue.

Added a preferInputActivator prop to Popover to allow better positioning of the overlay (#2754)

It's technically (also) a bugfix for #2752, but I have no issues moving it in the enhancements section. Thanks for the hint about linking to the issue. I wasn't aware that it should point to that.

dleroux

I made a suggestion on the Unreleased entry and the prop documentation. apart from that 🎩 worked perfectly. Thanks for doing this!

Co-authored-by: Robin Drexler <robin.drexler@shopify.com>

Co-Authored-By: Daniel Leroux <dleroux@users.noreply.github.com>

robin-drexler · 2020-02-20T15:03:51Z

@dleroux thanks for your review and suggestions. This PR should be good to go now. Should we wait for a second review or can we ? :)

ghost · 2020-02-24T14:09:17Z

🎉 Thanks for your contribution to Polaris React!

robin-drexler reviewed Feb 18, 2020

View reviewed changes

robin-drexler requested a review from dleroux February 18, 2020 21:22

robin-drexler mentioned this pull request Feb 18, 2020

[Popover] Allow more flexible Popover reference position calculation #2752

Closed

dleroux reviewed Feb 19, 2020

View reviewed changes

dleroux requested changes Feb 19, 2020

View reviewed changes

robin-drexler reviewed Feb 19, 2020

View reviewed changes

dleroux self-requested a review February 19, 2020 21:02

dleroux reviewed Feb 20, 2020

View reviewed changes

src/components/Popover/Popover.tsx Show resolved Hide resolved

dleroux reviewed Feb 20, 2020

View reviewed changes

dleroux approved these changes Feb 20, 2020

View reviewed changes

Attila Berki and others added 6 commits February 20, 2020 09:11

add preferInputActivator prop to Popover

5c8b5e6

Co-authored-by: Robin Drexler <robin.drexler@shopify.com>

Add changelog entry

63a667a

Co-authored-by: Robin Drexler <robin.drexler@shopify.com>

do not unnecessarily query for input activator

0b7cb4b

Co-authored-by: Robin Drexler <robin.drexler@shopify.com>

refactor popover test to not use legacy testing libs

71f0ec3

Co-authored-by: Robin Drexler <robin.drexler@shopify.com>

Updating test case due to removed testID

2b42a77

move fix to enhancements section in unreleased.md

48ae201

robin-drexler force-pushed the popover-prefer-input-activator branch from c10b430 to 48ae201 Compare February 20, 2020 14:15

add documentation for preferInputActivator prop in popover

7321dd6

Co-Authored-By: Daniel Leroux <dleroux@users.noreply.github.com>

robin-drexler merged commit 3b71196 into master Feb 24, 2020

robin-drexler deleted the popover-prefer-input-activator branch February 24, 2020 14:09

	const textFieldActivator = activator.querySelector('input');
	const activator = preferInputActivator ? activator.querySelector('input') : activatorProp


		const textFieldActivator = activator.querySelector('input');

		const activatorRect =

	const activatorRect =
	const activatorRect = getRectForNode(activator);

[Popover] Add preferInputActivator prop to Popover #2754

[Popover] Add preferInputActivator prop to Popover #2754

Uh oh!

Conversation

attila-berki commented Feb 18, 2020

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robin-drexler Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 56)

Files potentially affected (total: 57)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 61)

Files potentially affected (total: 0)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dleroux left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robin-drexler Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robin-drexler commented Feb 20, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dleroux left a comment

Choose a reason for hiding this comment

Uh oh!

robin-drexler commented Feb 20, 2020

Uh oh!

ghost commented Feb 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

robin-drexler Feb 18, 2020 •

edited

Loading

github-actions bot commented Feb 18, 2020 •

edited

Loading

robin-drexler Feb 19, 2020 •

edited

Loading