Add HorizontalActionList Component #511

chrismohr · 2018-08-22T20:20:53Z

Mocks: https://app.zeplin.io/project/5907d6451d2b06c6b36b2852/screen/5b7b17c4336cfa21f21efe87

Introduces a variation to ActionLink called "compact", which makes the link bold, has a smaller icon, and smaller gutter.

swese44 · 2018-08-22T21:12:21Z

what if ActionLink accepted additional props to support this? then ActionLinkList would just need a direction prop

chrismohr · 2018-08-23T16:51:58Z

@swese44 I like the idea of ActionLink accepting additional props to achieve the correct look, there are only two things that are different. The iconSize and gutterSize are smaller. I see couple of options to handle this. Right now I like adding a compact: boolean prop for this, we could also do a size: 'normal' | 'small', or just expose both iconSize and gutterSize. I think I like one of the first two options to limit options.

It would look like this:

    const gutterSize = compact ? GutterSize.XSMALL : GutterSize.SMALL;
    const iconSize = compact ? IconSize.SMALL : IconSize.MEDIUM;

The next decision might be around what to do with the count. We could add it to actionLink as an unlinkedSuffix or unlinkedText, but I think I like leaving it out of action link, and bringing it up a level, since it's layout (6px spacing) seems pretty specific to this case.
Then for ActionLinkList, I'm interested to hear how you imagined it working with the overflow menu. Would you pass in the overflow menu as an item? Or would you have it the menuButton in a column outside the ActionLinkList? Or would you add props/logic to ActionLink List to separate the items into the visible and overflow sets? Something else?

swese44 · 2018-08-28T21:35:06Z

src/components/ActionLink/ActionLink.types.ts

@@ -16,7 +16,12 @@ export interface BaseActionLinkProps extends BaseComponentProps {
 /**
 * The visible text.
 */
- text: string;
+ text: string | React.ReactNode;


Should this remain only string? it looks like you solved this with only string and we probably don't want to allow arbitrary markup here?

swese44 · 2018-08-28T21:40:23Z

src/components/MessageActionsList/MessageActionsList.tsx

+ * A set of links that dispaly in a horizontal list below a message post. When a maxVisibleItemCount
+ * prop is provided, items that exceed this value will show in a overflow menu.
+ */
+export class MessageActionsList extends React.Component<MessageActionsListProps & CustomizableComponentProps> {


it would be nice to name this something more generic so can be associated with a content item rather than Message. i'd expect to see yammer-web-components owning a <MessageActionsList> component which feeds the correct message-specific actions to this reusable UI component. maybe this could just be HorizontalActionList, or CompactActionList, ContentActionList... i don't think i'm coming up with any winners though

I agree. @cainhimself do you have any ideas or opinions here?

I've opted for HorizontalActionList.

swese44 · 2018-08-28T21:45:30Z

src/components/MessageActionsList/MessageActionsList.types.ts

+ maxVisibleItemCount?: number;
+}
+
+export type MessageActionsListItem = {


seems like we're duplicating ActionLink props, but losing the nuance of it's compound prop types (link or clickable). should we remove this interface and set line 9 to items: ActionItemProps[]?

Yeah, I agree this could be better. There are two things that led me to create the new interface here.

ActionLink has an optional ariaLabel, menuButton requires it.

Currently unlinkedText is rendered by the list component, not the ActionLink.

I'll try to figure something out.

so menuButton options have to be onclick handlers, not hyperlinks?

your call on this, whether it makes sense to extend ActionItemProps[] or not

Ok, I think I've improved it. Let me know what you think.

swese44 · 2018-08-29T20:07:53Z

src/components/AppContainer/AppContainer.tsx

@@ -15,7 +15,7 @@ export default class AppContainer extends React.Component<NestableBaseComponentP
 const { children, className, theme = defaultTheme } = this.props;
 const classNames = getClassNames({ theme });
 return (
- <Fabric>
+ <Fabric theme={theme}>


might need to double check this when you merge from master as Donnie's PR has been merged

Yeah, I see it's conflicting. I'll resolve it.

swese44 · 2018-08-30T23:37:15Z

is there anything blocking this merge so we can start the MC import?

chrismohr · 2018-08-31T16:50:26Z

@swese44, please feel free to merge this if it unblocks you.

@cainhimself is looking for changes to the menuButton spacing, but I'm happy to do this in a new PR next week.

swese44 · 2018-08-31T16:53:26Z

cool thanks

add post action link list

c547bb3

chrismohr requested review from BrendanMcK, jessemartin, richardtagger, swese44 and unindented as code owners August 22, 2018 20:20

Merge branch 'master' into postLinks

0443ad3

Chris Mohr and others added 7 commits August 23, 2018 10:05

refactoring

e945345

use correct icon color

81210b8

menuButton styling

286c6ed

update snapshots

72f52a7

add unit tests

de32bc6

rename to MessageActionsList

172084d

Merge branch 'master' into postLinks

6d9c7a3

chrismohr changed the title ~~[wip] add post action link list~~ Add MessageActionList Component Aug 28, 2018

chrismohr added the NEW COMPONENT label Aug 28, 2018

Chris Mohr added 2 commits August 28, 2018 09:05

update reference images

0627240

fix reference image

086e5d9

swese44 reviewed Aug 28, 2018

View reviewed changes

Chris Mohr added 2 commits August 29, 2018 09:58

fix vertical alignment

4eb75a5

update interface to support both links and clickable

749a9e2

chrismohr changed the title ~~Add MessageActionList Component~~ Add HorizontalActionList Component Aug 29, 2018

Chris Mohr added 4 commits August 29, 2018 10:18

use a more generic name

758b40c

remove stale reference images

4c38c42

add references with new name

c00aad0

add action states for open overflow menu

1d5a176

swese44 reviewed Aug 29, 2018

View reviewed changes

swese44 approved these changes Aug 29, 2018

View reviewed changes

Chris Mohr and others added 4 commits August 29, 2018 13:16

Merge branch 'master' into postLinks

657b13f

Merge branch 'master' into postLinks

6f30e3e

fix description

540393d

revert menuButton color override

c83c19b

Chris Mohr and others added 2 commits August 31, 2018 09:20

update snapshot

b1d3008

Merge branch 'master' into postLinks

1379ba9

swese44 merged commit b2c657c into master Aug 31, 2018

swese44 deleted the postLinks branch August 31, 2018 17:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add HorizontalActionList Component #511

Add HorizontalActionList Component #511

chrismohr commented Aug 22, 2018 •

edited

Loading

swese44 commented Aug 22, 2018

chrismohr commented Aug 23, 2018 •

edited

Loading

swese44 Aug 28, 2018

chrismohr Aug 29, 2018

swese44 Aug 28, 2018

chrismohr Aug 28, 2018

chrismohr Aug 29, 2018

swese44 Aug 28, 2018

chrismohr Aug 28, 2018 •

edited

Loading

swese44 Aug 28, 2018

swese44 Aug 29, 2018

chrismohr Aug 29, 2018

swese44 Aug 29, 2018

chrismohr Aug 29, 2018

swese44 commented Aug 30, 2018

chrismohr commented Aug 31, 2018

swese44 commented Aug 31, 2018

Add HorizontalActionList Component #511

Add HorizontalActionList Component #511

Conversation

chrismohr commented Aug 22, 2018 • edited Loading

swese44 commented Aug 22, 2018

chrismohr commented Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrismohr Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swese44 commented Aug 30, 2018

chrismohr commented Aug 31, 2018

swese44 commented Aug 31, 2018

chrismohr commented Aug 22, 2018 •

edited

Loading

chrismohr commented Aug 23, 2018 •

edited

Loading

chrismohr Aug 28, 2018 •

edited

Loading