Skip to content
Closed
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
5 changes: 5 additions & 0 deletions .changeset/famous-hairs-jam.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/polaris': minor
---

Updated top bar menu active state
Copy link
Member

@chloerice chloerice Apr 13, 2023

Choose a reason for hiding this comment

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

Suggested change
Updated top bar menu active state
- Updated `TopBar.UserMenu` active state styles
- Added the `truncate` prop to `ActionList.Item`
- Added support for setting an array of type ActionListSection[] on the `Topbar.UserMenu` `actions` prop

5 changes: 0 additions & 5 deletions polaris-react/src/components/ActionList/ActionList.scss
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,6 @@
svg {
fill: var(--p-color-icon-interactive);
}

&::before {
// stylelint-disable-next-line -- alignment for left tab style https://github.com/Shopify/polaris/pull/3619
@include list-selected-indicator;
}
}

&.destructive {
Expand Down
2 changes: 1 addition & 1 deletion polaris-react/src/components/ActionList/ActionList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ export function ActionList({
const sectionMarkup = finalSections.map((section, index) => {
return section.items.length > 0 ? (
<Section
key={section.title || index}
key={typeof section.title === 'string' ? section.title : index}
section={section}
hasMultipleSections={hasMultipleSections}
actionRole={actionRole}
Expand Down
54 changes: 47 additions & 7 deletions polaris-react/src/components/ActionList/components/Item/Item.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import React from 'react';
import {TickSmallMinor} from '@shopify/polaris-icons';

import {classNames} from '../../../../utilities/css';
import type {ActionListItemDescriptor} from '../../../../types';
Expand All @@ -11,6 +12,8 @@ import styles from '../../ActionList.scss';
import {handleMouseUpByBlurring} from '../../../../utilities/focus';
import {HorizontalStack} from '../../../HorizontalStack';
import {Box} from '../../../Box';
import {Tooltip} from '../../../Tooltip';
import {Truncate} from '../../../Truncate';

export type ItemProps = ActionListItemDescriptor;

Expand All @@ -33,6 +36,7 @@ export function Item({
ellipsis,
active,
role,
truncate = false,
}: ItemProps) {
const className = classNames(
styles.Item,
Expand Down Expand Up @@ -61,7 +65,15 @@ export function Item({
);
}

const contentText = ellipsis && content ? `${content}…` : content;
let contentText = null;
Copy link

@jamalsoueidan jamalsoueidan Apr 17, 2023

Choose a reason for hiding this comment

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

const contentText = ellipsis && content ? `${content}…` : (truncate ? truncateText(content) : content);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. Nesting a ternary operator is shorter, but the if else style is more readable to other devs.

Copy link

@jamalsoueidan jamalsoueidan Apr 17, 2023

Choose a reason for hiding this comment

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

You should consider using const most if not all places, and your condition is duplicated!

const contextText = content
if (ellipsis && content) {
  contentText = `${content}…`;
} else if (content && truncate) {
  contentText = truncateText(content);
} 

remove the else, and assign contextText = content as default.
but i still prefer one line, since its very short condition.

Copy link
Member

Choose a reason for hiding this comment

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

We prefer clarity over brevity, especially in this case because theres multiple props that influence what's rendered and one needs to take precedence.

Suggested change
let contentText = null;
let contentText = content
if (ellipsis) {
contentText = `${content}…`;
} else if (truncate) {
contentText = truncateText(content);
}

I do think that ellipsis should be deprecated in favor of the truncate prop you're adding @zakwarsame

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Working on it here 😄


if (ellipsis && content) {
contentText = `${content}…`;
} else if (content) {
contentText = truncate ? truncateText(content) : content;
} else {
contentText = content;
}

const contentMarkup = helpText ? (
<>
Expand All @@ -80,16 +92,28 @@ export function Item({
</span>
);

const suffixMarkup = suffix && (
<Box>
<span className={styles.Suffix}>{suffix}</span>
</Box>
);
let suffixMarkup: React.ReactNode | null = null;

if (active) {
suffixMarkup = (
<Box>
<span className={styles.Suffix}>
<Icon source={TickSmallMinor} />
</span>
</Box>
);
} else if (suffix) {
suffixMarkup = suffix && (
<Box>
<span className={styles.Suffix}>{suffix}</span>
</Box>
);
}
Comment on lines +95 to +111
Copy link
Contributor Author

@zakwarsame zakwarsame Apr 5, 2023

Choose a reason for hiding this comment

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

I tentatively made the suffix to always be TickSmallMinor when active is true following @chloerice's suggestion.
Will update if the consensus changes.


const textMarkup = <span className={styles.Text}>{contentMarkup}</span>;

const contentElement = (
<HorizontalStack blockAlign="center" gap="4">
<HorizontalStack blockAlign="center" gap="4" wrap={!truncate}>
{prefixMarkup}
{textMarkup}
{badgeMarkup}
Expand Down Expand Up @@ -134,3 +158,19 @@ export function Item({
</>
);
}

const truncateText = (text: string) => {
const trimmedText = text.trim();
return (
<Tooltip
content={trimmedText}
zIndexOverride={514}
preferredPosition="above"
hoverDelay={1000}
>
<Text truncate as="p" variant="bodyMd">
<Truncate>{trimmedText}</Truncate>
</Text>
</Tooltip>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,16 @@ describe('<Item />', () => {
onClick: null,
});
});

it('truncates content when truncate is true', () => {
const item = mountWithApp(
<Item
content="Test longer than usual string that probably overflows."
truncate
/>,
);
expect(item).toContainReactComponent(Text, {truncate: true});
});
});

function noop() {}
41 changes: 37 additions & 4 deletions polaris-react/src/components/TopBar/TopBar.stories.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
import React, {useCallback, useState} from 'react';
import type {ComponentMeta} from '@storybook/react';
import {ActionList, Frame, Icon, TopBar, Text} from '@shopify/polaris';
import {ArrowLeftMinor, QuestionMarkMajor} from '@shopify/polaris-icons';
import {ActionList, Frame, Icon, TopBar, Text, Avatar} from '@shopify/polaris';
import {
ArrowLeftMinor,
QuestionMarkMajor,
EditMinor,
TickSmallMinor,
SearchMinor,
} from '@shopify/polaris-icons';

export default {
component: TopBar,
Expand Down Expand Up @@ -49,15 +55,42 @@ export function Default() {
<TopBar.UserMenu
actions={[
{
items: [{content: 'Back to Shopify', icon: ArrowLeftMinor}],
items: [
{
active: true,
content: 'Jaded Pixel',
prefix: <Avatar size="small" name="Jaded Pixel" />,
suffix: <Icon source={TickSmallMinor} />,
truncate: true,
},
{
content: 'Jaded Pixel 2.0',
prefix: <Avatar size="small" name="Jaded Pixel 2.0" />,
truncate: true,
},
{
content: 'View all 3 stores',
prefix: <Icon source={SearchMinor} />,
},
],
},
{
items: [{content: 'Community forums'}],
},
{
items: [{content: 'Help Center'}],
},
{
items: [{content: 'Keyboard shortcuts'}],
},
{
title: 'Dharma Johnson',
items: [{content: 'Manage account'}, {content: 'Log out'}],
},
Comment on lines +86 to +89
Copy link
Member

Choose a reason for hiding this comment

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

Are all user menus going to have the store switcher eventually instead of the Stores action list item that links out to an identity page? If not, should this use case be documented as its own story since it's unique to merchants who have the store switcher?

Copy link
Contributor Author

@zakwarsame zakwarsame Apr 13, 2023

Choose a reason for hiding this comment

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

Yup! Our current project aims to roll out the new store switcher to all our merchants except Plus, who have a separate nav.

Copy link
Member

Choose a reason for hiding this comment

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

Ah okay! In that case, should we:

  • Make this a layout change internal to the component instead of adding a customActivator prop, since the pattern is being updated across the board?
  • Add an optional logo prop that renders a Thumbnail to the right of the name (instead of an avatar on the left) when provided, and make initials optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For future reference: moved

]}
name="Dharma"
detail="Jaded Pixel"
initials="D"
initials="JP"
open={isUserMenuOpen}
onToggle={toggleIsUserMenuOpen}
/>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
import React from 'react';

import type {IconableAction} from '../../../../types';
import {Avatar} from '../../../Avatar';
import type {AvatarProps} from '../../../Avatar';
import {MessageIndicator} from '../../../MessageIndicator';
import {Menu} from '../Menu';
import type {MenuProps} from '../Menu';
import {Text} from '../../../Text';
import type {ActionListProps} from '../../../ActionList';

import styles from './UserMenu.scss';

export interface UserMenuProps {
/** An array of action objects that are rendered inside of a popover triggered by this menu */
actions: {items: IconableAction[]}[];
actions: ActionListProps['sections'];
/** Accepts a message that facilitates direct, urgent communication with the merchant through the user menu */
message?: MenuProps['message'];
/** A string detailing the merchant’s full name to be displayed in the user menu */
Expand All @@ -29,6 +29,8 @@ export interface UserMenuProps {
open: boolean;
/** A callback function to handle opening and closing the user menu */
onToggle(): void;
/** A custom activator that can be used when the default activator is not desired */
customActivator?: React.ReactNode;
}

export function UserMenu({
Expand All @@ -41,10 +43,13 @@ export function UserMenu({
onToggle,
open,
accessibilityLabel,
customActivator,
}: UserMenuProps) {
const showIndicator = Boolean(message);

const activatorContentMarkup = (
const activatorContentMarkup = customActivator ? (
customActivator
) : (
<>
<MessageIndicator active={showIndicator}>
<Avatar
Expand Down
4 changes: 3 additions & 1 deletion polaris-react/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,8 @@ export interface ActionListItemDescriptor
suffix?: React.ReactNode;
/** Add an ellipsis suffix to action content */
ellipsis?: boolean;
/** Truncate action content */
truncate?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

@kyle Should ellipsis be depreecated in favor of truncate? It seems like the ellipsis prop is pointless as is and should have been doing actual truncation all this time since the content prop is string only (can't be passed in as truncated) 🤔

/** Whether the action is active or not */
active?: boolean;
/** Defines a role for the action */
Expand All @@ -212,7 +214,7 @@ export interface ActionListItemDescriptor

export interface ActionListSection {
/** Section title */
title?: string;
title?: string | React.ReactNode;
/** Collection of action items for the list */
items: readonly ActionListItemDescriptor[];
}
Expand Down