[Popover] Convert Popover to functional component #2386

dleroux · 2019-10-31T12:17:29Z

related to: #1995

WHY are these changes introduced?

I'm wanting to make use of a hook inside popover for my current project, so I took the opportunity to convert popover to a function components.

WHAT is this pull request doing?

Functionality remains the same. However there are changes.

ComponentDidMount and ComponentDidUpdate bundle into a single useEffect hook.
~~2. Use React.createElement to workaround creating jsx typing issue with the WrapperComponent~~
~~3. Tighten the type of activatorWrapper too so that it may only include strings of element names, e.g. 'button' and 'div' are valid, but not 'fff' or 'Button'~~ (breaking change)

thank you for those last two @BPScott!

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

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

import React, {useState, useCallback} from 'react';
import {Page, Popover, ActionList, Button} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      <PopoverWithActionListExample />
    </Page>
  );
}

function PopoverWithActionListExample() {
  const [popoverActive, setPopoverActive] = useState(false);
  const togglePopoverActive = useCallback(
    () => setPopoverActive((popoverActive) => !popoverActive),
    [],
  );
  const activator = (
    <Button onClick={togglePopoverActive} disclosure>
      More actions
    </Button>
  );
  return (
    <div style={{height: '250px'}}>
      <Popover
        active={popoverActive}
        activator={activator}
        onClose={togglePopoverActive}
      >
        <ActionList items={[{content: 'Import'}, {content: 'Export'}]} />
      </Popover>
    </div>
  );
}

🎩 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

github-actions · 2019-10-31T12:30:47Z

💦 Potential splash zone of changes introduced to src/**/*.tsx in this pull request:

Files modified	3
Files potentially affected	23

Details

All files potentially affected (total: 23)

📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

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

Files potentially affected (total: 23)

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

Files potentially affected (total: 0)

This comment automatically updates as changes are made to this pull request.
Feedback, troubleshooting: open an issue or reach out on Slack in #polaris-tooling.

dleroux · 2019-11-05T13:42:05Z

Just changing this to WIP because it will conflict with @AndrewMusgrave PR [#2255] so might as well wait to review.

AndrewMusgrave · 2019-11-05T15:05:02Z

src/components/Popover/Popover.tsx

@@ -26,7 +34,7 @@ export interface PopoverProps {
   * The element type to wrap the activator with
   * @default 'div'
   */
-  activatorWrapper?: string;
+  activatorWrapper?: HTMLOnlyJsxIntrinsicElementsKeys;


Running a type-check locally I don't have any problems keeping this a string?

It's fine to keep as a string from a typing perspective, but this is a little stricter. Before settling on the React.createElement approach I was trying to ensure you could only pass valid tagnames into this as while <div /> is fine, <divvvvvv /> is a type error and that caused problems with how we were trying to get types of <WrapperComponent>. This typing helped limit the activatorWrapper to only strings that were valid JSX. But in the end we didn't need that as createElement's first arg is typed as a string anyway.

dleroux · 2019-11-05T15:48:58Z

Ben added that to tighten the interface. Not sure if it's worth the impact.

The real issue was

<WrapperComponent testID="wrapper-component" ref={this.setActivator}>	
    {React.Children.only(this.props.activator)}	
    {portal}	
</WrapperComponent>

TypeScript wasn't liking this. The workaround was to use createElement instead:

return React.createElement(
    activatorWrapper,
    {ref: activatorContainer},
    React.Children.only(activator),
    portal,
  );

dleroux · 2019-11-07T13:24:52Z

I agree that tightening up the type is a good idea, but I would technically call this a breaking change. Would you agree?

dleroux · 2019-11-07T18:11:27Z

I've removed the tightened type. Maybe its something we can add in a major version but for now, it's probably not worth the change.

This is ready for review.

dleroux force-pushed the popover-to-function branch from f976091 to 8d74611 Compare October 31, 2019 12:20

dleroux requested review from BPScott and AndrewMusgrave October 31, 2019 12:20

dleroux changed the title ~~[WIP] [Popover] Convert Popover to functional component~~ [Popover] Convert Popover to functional component Oct 31, 2019

BPScott mentioned this pull request Oct 31, 2019

Post v4 cleanup work #1995

Closed

dleroux requested a review from chloerice November 5, 2019 13:18

dleroux changed the title ~~[Popover] Convert Popover to functional component~~ [WIP][Popover] Convert Popover to functional component Nov 5, 2019

AndrewMusgrave reviewed Nov 5, 2019

View reviewed changes

dleroux changed the title ~~[WIP][Popover] Convert Popover to functional component~~ [Popover] Convert Popover to functional component Nov 7, 2019

dleroux force-pushed the popover-to-function branch from 8d74611 to 10b1f4f Compare November 7, 2019 18:08

dleroux force-pushed the popover-to-function branch from 10b1f4f to 315f064 Compare November 7, 2019 19:50

dleroux requested a review from AndrewMusgrave November 7, 2019 19:52

dleroux force-pushed the popover-to-function branch 2 times, most recently from 895e0f3 to 8e92c72 Compare November 7, 2019 20:04

AndrewMusgrave approved these changes Nov 7, 2019

View reviewed changes

dleroux force-pushed the popover-to-function branch from 8e92c72 to ca05ddb Compare November 7, 2019 20:23

popover to functional

9ae691e

dleroux force-pushed the popover-to-function branch from ca05ddb to 9ae691e Compare November 7, 2019 20:24

dleroux merged commit 9633633 into master Nov 7, 2019

dleroux deleted the popover-to-function branch November 7, 2019 20:35

PabloVallejo temporarily deployed to production November 12, 2019 14:48 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Popover] Convert Popover to functional component #2386

[Popover] Convert Popover to functional component #2386

dleroux commented Oct 31, 2019 •

edited

Loading

github-actions bot commented Oct 31, 2019 •

edited

Loading

Files potentially affected (total: 0)

Files potentially affected (total: 23)

Files potentially affected (total: 0)

dleroux commented Nov 5, 2019

AndrewMusgrave Nov 5, 2019

BPScott Nov 6, 2019

dleroux commented Nov 5, 2019

dleroux commented Nov 7, 2019

dleroux commented Nov 7, 2019 •

edited

Loading

[Popover] Convert Popover to functional component #2386

[Popover] Convert Popover to functional component #2386

Conversation

dleroux commented Oct 31, 2019 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

github-actions bot commented Oct 31, 2019 • edited Loading

Details

Files potentially affected (total: 0)

Files potentially affected (total: 23)

Files potentially affected (total: 0)

dleroux commented Nov 5, 2019

AndrewMusgrave Nov 5, 2019

Choose a reason for hiding this comment

BPScott Nov 6, 2019

Choose a reason for hiding this comment

dleroux commented Nov 5, 2019

dleroux commented Nov 7, 2019

dleroux commented Nov 7, 2019 • edited Loading

dleroux commented Oct 31, 2019 •

edited

Loading

github-actions bot commented Oct 31, 2019 •

edited

Loading

dleroux commented Nov 7, 2019 •

edited

Loading