[Page] Add ReactNode as an accepted primaryAction prop value #3002

WillsonSmith · 2020-05-25T19:23:34Z

WHY are these changes introduced?

The current primaryAction on Page only allows for one button, I'm currently working on a project that has a need for more flexible primary actions.

WHAT is this pull request doing?

This PR maintains the existing primaryAction prop, while adding the option to pass a ReactNode instead for customizability.

What other approaches were considered?

We had considered building the API/UI for primaryAction to suit our specific need. We decided against this because we are not sure it's the right solution for everyone. This way if the provided option PrimaryAction doesn't cover every scenario, a custom option can be implemented.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

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

import React from 'react';

import {Page, Button} from '../src';

export function Playground() {
  return (
    <Page
      title="Playground"
      primaryAction={
        <Button
          primary
          connectedDisclosure={{
            accessibilityLabel: 'Other save actions',
            actions: [{content: 'Save as new'}],
          }}
        >
          Save
        </Button>
      }
    >
      {/* 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

ghost · 2020-05-25T19:23:38Z

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines.

github-actions · 2020-05-25T19:26:42Z

🟢 This pull request modifies 5 files and might impact 1 other files.

Details:

All files potentially affected (total: 1)

/src/components/Page/Page.tsx

📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Page/Page.tsx (total: 0)

Files potentially affected (total: 0)

📄 src/components/Page/README.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Page/components/Header/Header.tsx (total: 1)

Files potentially affected (total: 1)

/src/components/Page/Page.tsx

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

Files potentially affected (total: 0)

WillsonSmith · 2020-05-26T18:31:40Z

src/components/Page/Page.tsx

      title,
      buttons: transformActions(appBridge, {
-        primaryAction,
+        primaryAction: isPrimaryAction(primaryAction)


@BPScott Thoughts on how I'm handing the typing here & the helper function in general? Will probably ping you for review too, but was curious on your initial thoughts.

This seems alright at first glance. AppBridge integration is going away in v5. The PR has been open for an age and hopefully we'll get around to releasing it in 2 months, so I'd prioritize how to handle this stulf in non-AppBridge-land which you're doing anyway.

There's a fancy way to add optional object params

{ ...(isPrimaryAction(primaryAction) && {primaryAction}) }

BPScott · 2020-05-26T20:22:19Z

src/components/Page/components/Header/Header.tsx

-  const primary =
-    primaryAction &&
-    (primaryAction.primary === undefined ? true : primaryAction.primary);
+  function getPrimaryActionMarkup() {


Nested function definitions tend to feel odd to me - you need to go check for any closed over variables that appear in the scope. Can this be pulled out to the top level?

BPScott · 2020-05-26T20:24:26Z

src/components/Page/components/Header/Header.tsx

+    }
+
+    return (
+      <ConditionalWrapper


If you're writing if statements above then doing an early return of if (newDesignLanguage) { return content; } might be more readable than using ConditionalWrapper

BPScott

Two little things, but no major concerns. I'm gonna duck out of reviewing for now as I'll be on holiday till the 4th.

BPScott · 2020-05-27T22:25:22Z

src/components/Page/components/Header/Header.tsx

+export function isPrimaryAction(
+  x: PrimaryAction | React.ReactNode,
+): x is PrimaryAction {
+  return !React.isValidElement(x) && x !== undefined;


This feels a little odd to me - this condition is more "this is not a react node" rather than "this is a primary action", which is alright for now but might get confusing if we want to pass more sorts of things into this. Is there a way to check for action's shape rather than not ReactNode's shape?

I agree that it feels a little odd, the reason I did it this way is there are no required properties on primaryAction

BPScott · 2020-05-27T22:29:51Z

src/components/Page/components/Header/Header.tsx

  return layout.slots;
 }
+
+function getPrimaryActionMarkup(


A function that returns some JSX is a React component. What do you think to naming it and treating it as such? PrimaryActionMarkup() and then calling it like a react component above const primaryActionMarkup = primaryAction ? <PrimaryActionMarkup ... /> : null

Ah yeah this is the right decision, i'll do this.

AndrewMusgrave

Left a few comments but otherwise looks good to me 🎉 💯

AndrewMusgrave · 2020-05-28T17:33:08Z

src/components/Page/components/Header/tests/Header.test.tsx

+    it('renders a `ReactNode`', () => {
+      const primaryAction = <div>Hello</div>;
+
+      const header = mountWithAppProvider(
+        <Header {...mockProps} primaryAction={primaryAction} />,
+      );
+
+      expect(header.contains(primaryAction)).toBeTruthy();
+    });


Another cool way this could be handled is using react-testing matchers!

const PrimaryAction = () => null; const header = mountWithApp( <Header {...mockProps} primaryAction={<PrimaryAction />} />, ); expect(header).toContainReactComponent(PrimaryAction);

AndrewMusgrave · 2020-05-28T17:36:29Z

src/components/Page/Page.tsx

      title,
      buttons: transformActions(appBridge, {
-        primaryAction,
+        primaryAction: isPrimaryAction(primaryAction)


There's a fancy way to add optional object params

{ ...(isPrimaryAction(primaryAction) && {primaryAction}) }

AndrewMusgrave · 2020-05-28T17:41:12Z

src/components/Page/components/Header/Header.tsx

-        },
-      )}
-    </ConditionalWrapper>
+    <PrimaryActionMarkup primaryAction={primaryAction} />


We usually create an if statement rather than a component in situations like these, but the separation of concern is nice. Curious what others think?

I think this is fine.

AndrewMusgrave · 2020-05-28T17:43:46Z

src/components/Page/components/Header/Header.tsx

+  let content = primaryAction;
+  if (isPrimaryAction(primaryAction)) {


Nitpick: With an early return we don't have to wrap the entire component 😄

Suggested change

let content = primaryAction;

if (isPrimaryAction(primaryAction)) {

let content = primaryAction;

if (!isPrimaryAction(primaryAction)) return null

const primary = ...

I never want to return null with this component. This is wrapped in the condition because it changes content to be the button instead of primaryAction (which could be a React node). The ConditionalWrapper should always be returned with whatever content is as the children.

AndrewMusgrave · 2020-05-28T17:45:16Z

src/components/Page/components/Header/Header.tsx

+  const {isNavigationCollapsed} = useMediaQuery();
+  const {newDesignLanguage} = useFeatures();


Curious if anyone thinks we should pass these in as props?

Yeah I was curious about this too.

I'd leave it. It will be easier to clean-up and really doesn't become part of the API 🤷

dleroux

Looks good!

…m primary actions

alex-page · 2020-06-01T17:40:47Z

This is good to merge 🚀

ghost · 2020-06-01T17:48:22Z

🎉 Thanks for your contribution to Polaris React!

WillsonSmith changed the title ~~[WIP][Page] Add React.ReactNode as an accepted primaryAction prop value~~ [WIP][Page] Add ReactNode as an accepted primaryAction prop value May 25, 2020

WillsonSmith commented May 26, 2020

View reviewed changes

BPScott reviewed May 26, 2020

View reviewed changes

WillsonSmith force-pushed the page-primary-actions-section branch from b677a49 to cfa3aec Compare May 27, 2020 15:25

WillsonSmith changed the title ~~[WIP][Page] Add ReactNode as an accepted primaryAction prop value~~ [Page] Add ReactNode as an accepted primaryAction prop value May 27, 2020

WillsonSmith requested review from AndrewMusgrave, alex-page and dleroux May 27, 2020 17:13

WillsonSmith marked this pull request as ready for review May 27, 2020 17:14

WillsonSmith requested a review from BPScott May 27, 2020 18:53

BPScott reviewed May 27, 2020

View reviewed changes

AndrewMusgrave approved these changes May 28, 2020

View reviewed changes

dleroux approved these changes Jun 1, 2020

View reviewed changes

WillsonSmith force-pushed the page-primary-actions-section branch 2 times, most recently from 35d4ec6 to 18d21db Compare June 1, 2020 15:08

Make primaryAction accept PrimaryAction | React.ReactNode for custo…

090bc61

…m primary actions

WillsonSmith force-pushed the page-primary-actions-section branch from 5245417 to 090bc61 Compare June 1, 2020 17:10

alex-page approved these changes Jun 1, 2020

View reviewed changes

alex-page merged commit 2015bba into master Jun 1, 2020

alex-page deleted the page-primary-actions-section branch June 1, 2020 17:48

		let content = primaryAction;
		if (isPrimaryAction(primaryAction)) {

		const {isNavigationCollapsed} = useMediaQuery();
		const {newDesignLanguage} = useFeatures();

[Page] Add ReactNode as an accepted primaryAction prop value #3002

[Page] Add ReactNode as an accepted primaryAction prop value #3002

Uh oh!

Conversation

WillsonSmith commented May 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHY are these changes introduced?

WHAT is this pull request doing?

What other approaches were considered?

How to 🎩

🎩 checklist

Uh oh!

ghost commented May 25, 2020

Uh oh!

github-actions bot commented May 25, 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: 0)

Files potentially affected (total: 1)

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!

BPScott May 26, 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!

BPScott left a comment

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!

AndrewMusgrave left a comment

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!

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!

alex-page commented Jun 1, 2020

Uh oh!

ghost commented Jun 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

WillsonSmith commented May 25, 2020 •

edited

Loading

github-actions bot commented May 25, 2020 •

edited

Loading

BPScott May 26, 2020 •

edited

Loading