Skip to content

[Popover] Revert to type assertion to handle SVGElements #2827

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Mar 13, 2020
Merged

[Popover] Revert to type assertion to handle SVGElements #2827

merged 4 commits into from
Mar 13, 2020

Conversation

attila-berki
Copy link

Replacing type assertion with type check introduced a small issue in case of the <PopoverOverlay /> click handler.
If the target element is an SVGElement the descendent check is not even invoked and the Popover get's closed similarly when you click outside of it.

Reported issue:
https://github.com/Shopify/web/issues/24540

Partially reverting changes from #2638

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx: 
import React from 'react';
import {Page} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
    </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

@attila-berki attila-berki requested a review from tmlayton March 11, 2020 18:49
@github-actions
Copy link
Contributor

github-actions bot commented Mar 11, 2020 •
edited
Loading

🟡 This pull request modifies 3 files and might impact 57 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: 57)
📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 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)

@tmlayton
Copy link
Contributor

Worth adding a regression test?

@attila-berki
Copy link
Author

Worth adding a regression test?

Regression tests added, please let me know if something else is missing.

tmlayton
tmlayton approved these changes Mar 13, 2020
View reviewed changes
Copy link
Contributor

@tmlayton tmlayton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great and confirmed the test fails when running against master

@tmlayton tmlayton force-pushed the popover-remove-type-check-from-handle-click branch from 0faf534 to c51dc76 Compare March 13, 2020 23:14
@tmlayton tmlayton merged commit 77eccca into master Mar 13, 2020
@tmlayton tmlayton deleted the popover-remove-type-check-from-handle-click branch March 13, 2020 23:36
@tmlayton tmlayton mentioned this pull request Mar 14, 2020
Merged
@dhmacs
Copy link

dhmacs commented Mar 16, 2020

@attila-berki @tmlayton this doesn't seem to be fixed. clear button on TextField still makes the popover close

@attila-berki
Copy link
Author

@attila-berki @tmlayton this doesn't seem to be fixed. clear button on TextField still makes the popover close

Do you have an example or a branch I could checkout?

@dhmacs
Copy link

dhmacs commented Mar 16, 2020

Yes, I made this sandbox:

Edit f27cn

Click the textfield reset button and the popover closes.

Same thing happen when using an autocomplete and clicking on a choice

@attila-berki
Copy link
Author

attila-berki commented Mar 18, 2020 •
edited
Loading

Yes, I made this sandbox:

Edit f27cn
Click the textfield reset button and the popover closes.

Same thing happen when using an autocomplete and clicking on a choice

Hey @Macs91 👋 It looks to me that this is not related to the SVGElement type check, if you remove the clear button click handler the Popover remains open.

I would suggest to check the click handler implementation of the clear button and compare it to the click handler of the increment / decrement arrows.
Opening a new issue would also be a good first step.

athornburg pushed a commit to athornburg/polaris-react that referenced this pull request Apr 15, 2020
@athornburg
[Popover] Revert to type assertion to handle SVGElements (Shopify#2827)
17b7e08 
* Revert to type assertion to handle SVGElements

* Add regression tests

Co-authored-by: Tim Layton <tim.layton@shopify.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@tmlayton tmlayton tmlayton approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@attila-berki @tmlayton @dhmacs