Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions UNRELEASED.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@

### Enhancements

- Added `ReactNode` as an accepted prop type to `primaryAction` on the `Page` component ([#3002](https://github.com/Shopify/polaris-react/pull/3002))
- Added a `fullWidth` prop to `EmptyState` to support full width layout within a content context ([#2992](https://github.com/Shopify/polaris-react/pull/2992))
- Added an `emptyState` prop to `ResourceList` to support in context empty states in list views ([#2569](https://github.com/Shopify/polaris-react/pull/2569))

### Bug fixes

### Documentation
Expand Down
4 changes: 2 additions & 2 deletions src/components/Page/Page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
WithAppProviderProps,
} from '../../utilities/with-app-provider';

import {Header, HeaderProps} from './components';
import {Header, HeaderProps, isPrimaryAction} from './components';
import styles from './Page.scss';

export interface PageProps extends HeaderProps {
Expand Down Expand Up @@ -166,7 +166,7 @@ class PageInner extends React.PureComponent<ComposedProps, never> {
return {
title,
buttons: transformActions(appBridge, {
primaryAction,
...(isPrimaryAction(primaryAction) && {primaryAction}),
secondaryActions,
actionGroups,
}),
Expand Down
26 changes: 26 additions & 0 deletions src/components/Page/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,32 @@ Use for building any page on iOS.

<!-- /content-for -->

### Page with custom primary action

<!-- example-for: web -->

Use to create a custom primary action.

```jsx
<Page
breadcrumbs={[{content: 'Settings', url: '/settings'}]}
title="General"
primaryAction={
<Button
primary
connectedDisclosure={{
accessibilityLabel: 'Other save actions',
actions: [{content: 'Save as new'}],
}}
>
Save
</Button>
}
>
<p>Page content</p>
</Page>
```

### Page without primary action in header

<!-- example-for: web -->
Expand Down
66 changes: 44 additions & 22 deletions src/components/Page/components/Header/Header.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export interface HeaderProps extends TitleProps {
/** Adds a border to the bottom of the page header (stand-alone app use only) */
separator?: boolean;
/** Primary page-level action */
primaryAction?: PrimaryAction;
primaryAction?: PrimaryAction | React.ReactNode;
/** Page-level pagination (stand-alone app use only) */
pagination?: PaginationDescriptor;
/** Collection of breadcrumbs */
Expand All @@ -56,6 +56,12 @@ export interface HeaderProps extends TitleProps {
additionalNavigation?: React.ReactNode;
}

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

export function Header({
title,
subtitle,
Expand Down Expand Up @@ -116,28 +122,8 @@ export function Header({
/>
);

const primary =
primaryAction &&
(primaryAction.primary === undefined ? true : primaryAction.primary);

const primaryActionMarkup = primaryAction ? (
<ConditionalWrapper
condition={newDesignLanguage === false}
wrapper={(children) => (
<div className={styles.PrimaryActionWrapper}>{children}</div>
)}
>
{buttonsFrom(
shouldShowIconOnly(
newDesignLanguage,
isNavigationCollapsed,
primaryAction,
),
{
primary,
},
)}
</ConditionalWrapper>
<PrimaryActionMarkup primaryAction={primaryAction} />
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine.

) : null;

const actionMenuMarkup =
Expand Down Expand Up @@ -228,6 +214,42 @@ export function Header({
);
}

function PrimaryActionMarkup({
primaryAction,
}: {
primaryAction: PrimaryAction | React.ReactNode;
}) {
const {isNavigationCollapsed} = useMediaQuery();
const {newDesignLanguage} = useFeatures();
Comment on lines +222 to +223
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I was curious about this too.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

let content = primaryAction;
if (isPrimaryAction(primaryAction)) {
Comment on lines +224 to +225
Copy link
Member

Choose a reason for hiding this comment

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

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 = ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

const primary =
primaryAction.primary === undefined ? true : primaryAction.primary;

content = buttonsFrom(
shouldShowIconOnly(
newDesignLanguage,
isNavigationCollapsed,
primaryAction,
),
{
primary,
},
);
}

return (
<ConditionalWrapper
condition={newDesignLanguage === false}
wrapper={(children) => (
<div className={styles.PrimaryActionWrapper}>{children}</div>
)}
>
{content}
</ConditionalWrapper>
);
}

function shouldShowIconOnly(
newDesignLanguage: boolean,
isMobile: boolean,
Expand Down
11 changes: 11 additions & 0 deletions src/components/Page/components/Header/tests/Header.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
} from 'components';
// eslint-disable-next-line no-restricted-imports
import {mountWithAppProvider} from 'test-utilities/legacy';
import {mountWithApp} from 'test-utilities';

import type {LinkAction} from '../../../../../types';
import {Header, HeaderProps} from '../Header';
Expand Down Expand Up @@ -98,6 +99,16 @@ describe('<Header />', () => {
const expectedButton = buttonsFrom(primaryAction, {primary: false});
expect(header.contains(expectedButton)).toBeTruthy();
});

it('renders a `ReactNode`', () => {
const PrimaryAction = () => null;

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

expect(header).toContainReactComponent(PrimaryAction);
});
});

describe('pagination', () => {
Expand Down