[ActionMenu] Add support for explicit item order #2057

chloerice · 2019-08-28T21:33:28Z

WHY are these changes introduced?

The Orders team needs to be able to specify the order of their Page actions because one of their actions varies from being a secondaryAction or an actionGroup depending on the status of the order.

WHAT is this pull request doing?

Adds support for overriding the order of actions in the action menu.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

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

import React from 'react';
import {Page, Thumbnail, Badge} from '../src';

export function Playground() {
  return (
    <Page
      breadcrumbs={[{content: 'Products', url: '/products'}]}
      title="3/4 inch Leather pet collar"
      titleMetadata={<Badge status="success">Paid</Badge>}
      subtitle="Perfect for any pet"
      thumbnail={
        <Thumbnail
          source="https://burst.shopifycdn.com/photos/black-leather-choker-necklace_373x@2x.jpg"
          alt="Black leather pet collar"
        />
      }
      primaryAction={{content: 'Save', disabled: true}}
      secondaryActions={[
        {
          content: 'Duplicate',
          accessibilityLabel: 'Secondary action label',
        },
        {content: 'View on your store'},
      ]}
      actionGroups={[
        {
          index: 0,
          title: 'Promote',
          actions: [
            {
              content: 'Share on Facebook',
              accessibilityLabel: 'Individual action label',
              onAction: () => {},
            },
          ],
        },
      ]}
      pagination={{
        hasPrevious: true,
        hasNext: true,
      }}
      separator
    >
      <p>Page content</p>
    </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

danrosenthal

I mentioned this IRL, but I think it would be interesting to explore an API that maps to the flexbox order property. As you said, this could even map directly to that property in the implementation, using flexbox to reorder these as specified. This would also probably simplify the code a good bit, and might be a positive for accessibility.

chloerice · 2019-09-05T21:50:07Z

Quick update: This was initially done in and following an adhoc pairing session and will be prioritized next week.

chloerice · 2019-09-05T21:56:28Z

I mentioned this IRL, but I think it would be interesting to explore an API that maps to the flexbox order property. As you said, this could even map directly to that property in the implementation, using flexbox to reorder these as specified. This would also probably simplify the code a good bit, and might be a positive for accessibility.

I think there was a mixup when I spoke briefly about this work during show and tell. The reason I named the property index vs order is because both secondaryActions and actionGroups are arrays of actions, and in order to override their order they are combined into a single array of actions and any with an index specified are moved into that place in the list.

The index provided maps to the order in the overall list of actions below the title. I'd mentioned that I initially called the property order, but changed it to index because I didn't want there to be confusion around the way the order is overridden under the hood. This is because the flexbox order property has accessibility issues that we don't want to propagate.

github-actions · 2019-10-09T20:51:12Z

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

Files modified	7
Files potentially affected	136

Details

All files potentially affected (total: 136)

📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/ActionMenu/ActionMenu.tsx (total: 2)

Files potentially affected (total: 2)

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

Files potentially affected (total: 0)

🧩 src/components/ActionMenu/tests/utilities.test.ts (total: 0)

Files potentially affected (total: 0)

🧩 src/components/ActionMenu/utilities.ts (total: 3)

Files potentially affected (total: 3)

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

Files potentially affected (total: 1)

/src/components/Page/Page.tsx

🧩 src/types.ts (total: 136)

Files potentially affected (total: 136)

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.

brandon-yetman · 2020-01-08T18:16:01Z

Hey @chloerice, any update on when this will be ready to go?

chloerice · 2020-01-16T22:51:56Z

@brandon-yetman This is ready to go! Codecov is still fighting me, though. @AndrewMusgrave looking at the diff in codecov vs the tests I have here, do you see any obvious holes in tests? It's driving me nuts 😣

brandon-yetman · 2020-01-17T15:00:35Z

Thanks so much for your hard work on this @chloerice !!

AndrewMusgrave · 2020-01-17T16:48:19Z

src/components/ActionMenu/ActionMenu.tsx

    const {activeMenuGroup} = this.state;
    const menuActions = [...actions, ...groups];

-    if (menuActions.length === 0) {


This branch was never executing because we're returning early in the render method

AndrewMusgrave · 2020-01-17T16:49:15Z

src/components/ActionMenu/utilities.ts

+  index: number;
+};
+
 export function sortAndOverrideActionOrder(


Here we're filtering out the descriptors without index, so we're never hitting the edges cases below.

AndrewMusgrave · 2020-01-17T16:49:34Z

Here we're not hitting the default properties (indexA = 0)

Here we're not hitting the else statement

Here we're never exiciting this branch

AndrewMusgrave · 2020-01-17T16:54:05Z

That should bump codecov to 100% but I'll check back after lunch. Here're my changes, I left some comments. Let me know if you have any questions 😄

AndrewMusgrave · 2020-01-17T17:31:10Z

💯

chloerice · 2020-01-17T22:51:02Z

That should bump codecov to 100% but I'll check back after lunch. Here're my changes, I left some comments. Let me know if you have any questions 😄

Thank you so much @AndrewMusgrave!!! 🙏 🙌

brandon-yetman

🎩 and code looks great!

One detail of note is that the rollup menu (i.e. on a mobile display) does not take into account the explicit item order, but this is a) not explicitly required for the use-case that brought about this PR, and b) potentially undesireable.

Thanks again @chloerice !

brandon-yetman · 2020-01-21T20:06:04Z

src/components/ActionMenu/utilities.ts

+  ];
+
+  sortedActionsWithOverrides.forEach((action) => {
+    overriddenActions.splice(action.index, 0, action);


Very elegant solution 👍

AndrewMusgrave

LGTM 🎩 ✅ Left a few comments about type safety. We also should add a readme example, but it shouldn't delay this getting shipped 😄

AndrewMusgrave · 2020-01-21T21:26:59Z

UNRELEASED.md

@@ -1,60 +1,19 @@
 # Unreleased changes



A rebase might be needed 😄

AndrewMusgrave · 2020-01-21T21:30:58Z

src/components/ActionMenu/ActionMenu.tsx

-      ? groups.map(({title, ...rest}, index) => (
+    const actionMarkup = overriddenActions.map((action, index) => {
+      if (Object.hasOwnProperty.call(action, 'title')) {
+        const {title, actions, ...rest} = action as MenuGroupDescriptor;


The in keyword works as a type guard for narrowing.

if ('title' in action) { const {title, actions, ...rest} = action;

AndrewMusgrave · 2020-01-21T21:33:39Z

src/components/ActionMenu/ActionMenu.tsx

-        {groupsMarkup}
-      </div>
-    ) : null;
+      const {content, ...rest} = action as MenuActionDescriptor;


With the guard above we can remove this cast as well!

AndrewMusgrave · 2020-01-21T21:42:39Z

src/components/ActionMenu/utilities.ts

+) {
+  const actionsWithOverrides = actions.filter(
+    (action) => action.index !== undefined,
+  ) as MenuDescriptorWithIndex[];


If we wanted type safety we could approach it like this.

... const actionsWithOverrides = actions.filter( (action) => action.index !== undefined, ); // Cast removed if (isMenuDescriptionWithIndex(actionsWithOverrides)) { // reversed logic ... } return actions; } function isMenuDescriptionWithIndex( // narrowing function actions: (MenuActionDescriptor | MenuGroupDescriptor)[], ): actions is MenuDescriptorWithIndex[] { return actions.length !== 0; }

chloerice · 2020-01-22T18:10:00Z

🎩 and code looks great!

One detail of note is that the rollup menu (i.e. on a mobile display) does not take into account the explicit item order, but this is a) not explicitly required for the use-case that brought about this PR, and b) potentially undesireable.

Thanks again @chloerice !

Thanks @brandon-yetman! The rollup menu wasn't included in these changes because there would need to be a redesign of ActionList. Right now it always renders the sections (ActionMenu.groups on large screens) last so the items (ActionMenu.items on large screens) don't get lost in the lack of hierarchy. I'll make an issue and get some design feedback and ship this for now so it doesn't block you and your team.

brandon-yetman · 2020-01-22T18:23:29Z

Yeah I figured it's out of scope for this PR anyways, just wanted to make sure it was noted. Thanks 🙌

danrosenthal reviewed Aug 29, 2019

View reviewed changes

chloerice force-pushed the action-order-override branch from 6b3d749 to 8ace217 Compare October 9, 2019 20:45

chloerice force-pushed the action-order-override branch from 8ace217 to e104532 Compare October 11, 2019 19:44

chloerice force-pushed the action-order-override branch 7 times, most recently from 38cf66f to 5bfd938 Compare October 23, 2019 01:01

brandon-yetman mentioned this pull request Jan 8, 2020

[Page] Allow SecondaryActions to be a dropdown (or allow control of SecondaryActions/ActionGroup Ordering) #1762

Closed

chloerice force-pushed the action-order-override branch 7 times, most recently from 82c6e6f to eb4e39c Compare January 9, 2020 21:55

chloerice force-pushed the action-order-override branch 5 times, most recently from f3b37c0 to 7668662 Compare January 16, 2020 22:07

chloerice changed the title ~~[WIP][ActionMenu] Add support for explicit item order~~ [ActionMenu] Add support for explicit item order Jan 16, 2020

chloerice requested a review from AndrewMusgrave January 16, 2020 22:49

chloerice marked this pull request as ready for review January 16, 2020 22:52

AndrewMusgrave reviewed Jan 17, 2020

View reviewed changes

brandon-yetman approved these changes Jan 21, 2020

View reviewed changes

AndrewMusgrave approved these changes Jan 21, 2020

View reviewed changes

chloerice force-pushed the action-order-override branch from 5c84213 to 6ada143 Compare January 22, 2020 18:10

chloerice and others added 4 commits January 22, 2020 15:39

[ActionMenu] Add support for explicit item order

39df8d4

working on test coverage...

a29ccc1

Bump codecov

4d0308d

fix unrealeased

934e587

chloerice force-pushed the action-order-override branch from 574a5ea to 934e587 Compare January 22, 2020 20:39

chloerice merged commit aa3dfba into master Jan 22, 2020

chloerice deleted the action-order-override branch January 22, 2020 20:48

dleroux temporarily deployed to production January 27, 2020 13:38 Inactive

[ActionMenu] Add support for explicit item order #2057

[ActionMenu] Add support for explicit item order #2057

Uh oh!

Conversation

chloerice commented Aug 28, 2019 • 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?

How to 🎩

🎩 checklist

Uh oh!

danrosenthal left a comment

Choose a reason for hiding this comment

Uh oh!

chloerice commented Sep 5, 2019

Uh oh!

chloerice commented Sep 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Files potentially affected (total: 0)

Files potentially affected (total: 2)

Files potentially affected (total: 0)

Files potentially affected (total: 0)

Files potentially affected (total: 3)

Files potentially affected (total: 1)

Files potentially affected (total: 136)

Uh oh!

brandon-yetman commented Jan 8, 2020

Uh oh!

chloerice commented Jan 16, 2020

Uh oh!

brandon-yetman commented Jan 17, 2020

Uh oh!

AndrewMusgrave Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

AndrewMusgrave Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

AndrewMusgrave commented Jan 17, 2020

Uh oh!

AndrewMusgrave commented Jan 17, 2020

Uh oh!

AndrewMusgrave commented Jan 17, 2020

Uh oh!

chloerice commented Jan 17, 2020

Uh oh!

brandon-yetman left a comment

Choose a reason for hiding this comment

Uh oh!

brandon-yetman Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

AndrewMusgrave left a comment

Choose a reason for hiding this comment

Uh oh!

AndrewMusgrave Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

AndrewMusgrave Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

AndrewMusgrave Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

AndrewMusgrave Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

chloerice commented Jan 22, 2020

Uh oh!

brandon-yetman commented Jan 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

chloerice commented Aug 28, 2019 •

edited

Loading

chloerice commented Sep 5, 2019 •

edited

Loading

github-actions bot commented Oct 9, 2019 •

edited

Loading