Create DrawerFooter component with dropdown toggle for primary action

[eric2523]

Description

Us at rx-fast want to extend functionality of the DrawerFooter to also allow for a DropdownToggle as it's primary action. This adds a new additionalPrimaryActions prop that is the interface for rendering the individual DropdownItems.

The DrawerFooter now conditionally renders a single primary action or dropdown toggle depending on additionalPrimaryActions

This also adds the ability to add a trailing icon to the DropdownItem.

Screenshot

[eric2523]

with another thought it might be better to use the <DropdownToggle /> component here and just have a primaryActionText prop. Forces this component to stay true to being a dropdown toggle

[gabescarbrough]

Yeah I think avoiding render functions is a good idea

[kyleshike]

What do you think about just extending DrawerDropdownFooter to be able to accept either a primary action or a primary dropdown rather than a new component with its own interface? I think that it might help reduce complexity in rails-server (eg: number of components to import for a drawer, etc)

If could be something like:

DrawerFooter.propTypes = {
  // ...etc
  primaryActionDropdownItems: propTypes.arrayOf(proptypes.shape({
    text: propTypes.node.isRequired,
    onClick: propTypes.func.isRequired,
  })),
};

Then in the component, just do something like:

    {onPrimaryAction && (
        <Button
          disabled={primaryActionDisabled}
          isLoading={isPrimaryActionLoading}
          leadingIcon={primaryActionIcon}
          loadingText={primaryActionLoadingText}
          type="button"
          variant={primaryActionVariant}
          onClick={onPrimaryAction}
        >
          {primaryActionText}
        </Button>
      )}
      {primaryActionDropdownItems && (
        <Dropdown>
          <DropdownToggle variant={primaryActionVariant}>
            {primaryActionText}
          </DropdownToggle>
          <DropdownMenu>
            {primaryActionDropdownItems.map((item) =>
              <DropdownItem href="#" onClick={item.onClick}>{item.text}</DropdownItem>)}
          </DropdownMenu>
        </Dropdown>
      )}

Curious what y'all think

[eric2523]

Yea I was a bit torn on this. My OG thought process was if we add an additional prop to DropdownFooter we start getting into the world where we are telling the component what to do vs getting the thing we need off the bat. Buuuttt that was based off my idea that either the Dropdown or PrimaryAction would render and not both. With that I think the first question to answer is if it makes sense to allow the footer to have both types of buttons displayed together. That will decide how opinionated we want this thing to be.

Imo I feel like we'd want to enforce a secondary, primary and not tertiary button? At least for now it seems like a lot of our drawers are forms when they have these actions and so idk if i see a picture/world of having both.

@kyleshike

[kyleshike]

Yea totally agreed that this seems like something we'd want as an either/or.

As I was writing out what it'd look like, I kept thinking how much cooler it'd be in Typescript where we could enforce that there be primary action or primary dropdown but not both, without requiring a separate component to describe the behavior. What do you think about converting the footer to TS, and adding the types to support that restriction?

[gabescarbrough]

What do you think about just extending DrawerDropdownFooter to be able to accept either a primary action or a primary dropdown rather than a new component with its own interface? I think that it might help reduce complexity in rails-server (eg: number of components to import for a drawer, etc)

If allowing multiple primary actions was a requirement at the time I was deciding the props for the Drawer components I think I would have approached it similar to what you outlined instead of taking one primary action. If the array only had one action then you could display it as a button, and if there was more than one you could display it as a select.

DrawerFooter.propTypes = {
  // ...etc
  primaryActions: propTypes.arrayOf(proptypes.shape({
    text: propTypes.node.isRequired,
    onClick: propTypes.func.isRequired,
  })),
};

I tried to keep the Drawer's child components decoupled enough that you could write a custom version of any of them easily. So having a different DrawerFooter doesn't bother me, but I do get the argument that this is very similar to the existing footer so it might make sense to just have one.

[gabescarbrough]

It does kind of bug me that this isn't a split button. My expectation would be that clicking "Save" would save and clicking the down-arrow icon would show additional options. Having it as a dropdown feels off to me, but we don't have a split button right now so that would be something else we'd have to add first.

[gabescarbrough]

Imo I feel like we'd want to enforce a secondary, primary and not tertiary button? At least for now it seems like a lot of our drawers are forms when they have these actions and so idk if i see a picture/world of having both.

For what its worth, the original use-case for the Drawer (bulk emails) has the preview button over on the left as a tertiary button which was done by passing another Button as children to the DrawerFooter. The original idea was that anything you needed to add over to the left could be passed as children and be more use-case specific whereas the primary and secondary buttons would generally always be the same. If you needed something fully custom you could build your own DrawerFooter and use it with the Drawer. I still prefer tertiary items over to the left and am not a big fan of dropdowns used like this but I understand that folks think it looks cleaner.

[eric2523]

If the array only had one action then you could display it as a button, and if there was more than one you could display it as a select.

Hmm imo this feels like a primitive obsession smell where we are trying to using an array's length to define a type and use it to control a lot of logic. Which bc of that I do lean a towards kyle's approach with TS and using type predicates to distinguish between the two diff action buttons.

For what its worth, the original use-case for the Drawer (bulk emails) has the preview button over on the left as a tertiary button which was done by passing another Button as children to the DrawerFooter.

Yea on the new workspace side of things we are trying to align the way we handle saves and previews. I'm not sure if this was discussed amongst the designers whether we wanna move behavior towards the dropdown tho

[gabescarbrough]

Hmm imo this feels like a primitive obsession smell where we are trying to using an array's length to define a type and use it to control a lot of logic.

It doesn't feel like a lot of logic to me. Either it is one action and you display it as one thing (a button) or it is multiple actions and you display it as multiple grouped things (a select). It's not so much defining a type, as displaying correctly for the amount of actions given. It doesn't matter much now though because switching to that prop interface would be a breaking change so its a no-go anyway.

I'm not opposed to enforcing one or the other with TS. Then it is clear that you're meant to use a button or a select for the primary action and not both. Having the necessary props for either feels kind of ugly to me. That's what makes me wish you could just provide an array of actions and have the component worry about how to display it. But having props for either gets you back to having one DrawerFooter component so I'm not opposed if that is a priority for folks.

Yea on the new workspace side of things we are trying to align the way we handle saves and previews. I'm not sure if this was discussed amongst the designers whether we wanna move behavior towards the dropdown tho

Yeah it was. I made the case that I prefer the tertiary button over to the side but I'm the outlier 👀

[natkasman]

Scoooching in here to add some context and rationale behind this!

I 1000% agree that a split button is more appropriate for this than a dropdown.

So currently we have:
(1) Preview participant view for scheduling
(2) Preview screener
(3) Preview emails in workspace
(4) Preview email in Hub table

While all different, they are similar in requiring the user to "Save" before previewing. Currently, we do what was suggested, and display the Preview button on the left. We got feedback that it was unclear what the preview experience was going to be—were users going to see what they typed pre-save, or did they need to save first in order for the preview to work?

We changed the button language to "Save and send preview" but it's quite lengthy for button text, and makes it more obvious that there are 2 "Save"-related actions in the action bar. Given that our app is already quite text-heavy, we thought this approach would be more win-win because it makes it obvious for users that you need to Save first before you can preview, and minimizes the options that users can take.

Wrt the question of "are we doing this for all previews" - I think it makes sense for the experience to be consistent (I think the above 4 are the only preview experiences we have, but lmk if i'm missing anything). If I'm missing a preview experience, and it doesn't make sense to do this in that scenario, happy to explore!

[jasonbasuil]

It does kind of bug me that this isn't a split button. My expectation would be that clicking "Save" would save and clicking the down-arrow icon would show additional options. Having it as a dropdown feels off to me, but we don't have a split button right now so that would be something else we'd have to add first.

➕ fwiw haven't looked too close but looks like DropdownToggle supports a split button https://react-bootstrap.netlify.app/docs/components/dropdowns#split-button-dropdowns

[eric2523]

Having the necessary props for either feels kind of ugly to me. That's what makes me wish you could just provide an array of actions and have the component worry about how to display it.

Mm i see wym. Like you gotta know that potentially prop A can't be used with prop B or something like that. Im curious if we pass an array of actions how will that alleviate the "either this prop or that prop" problem? Expose only the props/behaviors that applies to both a Button or DropdownToggle? If so, what if theres a control that only DropdownToggle uses but is important enough that we wanna expose it.

[gabescarbrough]

After thinking about this some more I think the way to go is

DrawerFooter.propTypes = {
  // ...etc
  isPrimaryActionLoading: propTypes.bool,
  primaryActionDisabled: propTypes.bool,
  primaryActionIcon: propTypes.object,
  primaryActionLoadingText: propTypes.string,
  primaryActionText: propTypes.string,
  primaryActionVariant: propTypes.string,
  onPrimaryAction: propTypes.func,
  additionalPrimaryActions: propTypes.arrayOf(proptypes.shape({
    text: propTypes.node.isRequired,
    onClick: propTypes.func.isRequired,
  })),
};

If you provide additionalPrimaryActions then the primary action is displayed as a split button with the additional actions in the dropdown. If you don't then it is displayed as a button like now. The DrawerFooter owns how the button is displayed as opposed to taking a render function.

This is backwards compatible. The existing props relate to the primary button can still work the same. isPrimaryActionLoading, primaryActionDisabled, etc are relevant whether there are additional actions or not. It is explicit which action is intended for the button vs the select sections of the split button.

This pattern doesn't require props that are exclusive of each other. Converting to TS would be great but I think having props that don't work with each other is confusing.

We don't need to have multiple DrawerFooter components and I think its pretty intuitive what additionalPrimaryActions is for, especially if you have seen a split button example.

Expose only the props/behaviors that applies to both a Button or DropdownToggle? If so, what if theres a control that only DropdownToggle uses but is important enough that we wanna expose it.

Expose only the props we want devs to use. Since the goal here is consistency it's actually better to not make this too flexible. The existing props should play nice with either a button or a split button. Some of the existing props we should probably actually remove in a future major version, like primaryActionVariant was intended to allow you to use a danger button for a destructive action or similar and we don't recommend that anymore.

[eric2523]

One thing i'm thinking about is the onPrimaryAction prop for a toggle and whether we should make that exclusive but I can see the benefit of having some tracking passed to that primary action which is okay. Like if you choose to do some weird behavior thats on you lol.

I think I like it. This checks off:

• restricting the type of primary button that renders.
• Having one component
• Not falling into primitive obsession as the additionalPrimaryActions is like the type that drives render logic instead of the array length
• Not having exclusive props exposed that only pertains to one or the other

This is great i like it and i'm down to move on with this solution!

[kyleshike]

I might be misreading this, but what happens when both onPrimaryAction is present and isDropdownToggle is true? It looks like it won't render the primary action in that case. Is that intentional?

[gabescarbrough]

@eric2523 my intention was that when additionalPrimaryActions is passed then the primary action should be a split button, not a dropdown. I think having a typical dropdown for a primary action is confusing. I don't think I've ever seen that in an app before. Non-split dropdowns are better suited to menus of actions as opposed to primary actions.

@kyleshike if the primary action is a split button then onPrimaryAction is what is called when you click the primary action, and the additionalPrimaryActions onClick functions would be called for the additional actions.

Using this random split button example I found on NN/g, clicking "Insert" would call onPrimaryAction. Clicking the downward arrow then clicking "Insert above" would call the onClick associating with that action.

As Jason pointed out React Bootstrap's Dropdown component supports split buttons so ours (which wraps it) should be able to as well: https://react-bootstrap.netlify.app/docs/components/dropdowns/#split-button-dropdowns

My preference would be to add a SplitButton component so we could use it elsewhere easily as well. I could see an argument for supporting split buttons as part of Button or Dropdown but it's probably more discoverable if it is its own thing. Also the props could be neater.

[kyleshike]

I agree that a split button probably makes the most sense here. @eric2523 @natkasman is there timeline pressure on this? We'll want to be considerate of this spiraling out too far, but also good to get it right 👍

[natkasman]

I think we preferably want to get this in soon bc we need this for the "Preview participant's view" functionality for Automated scheduling. But I agree that a split button makes the most sense here and will be more flexible for future usage. If it doesn't take too long to add and use that instead, I'm on board.

[gabescarbrough]

I don't think it would be a big lift to switch to using a split button but I also get that balancing responding to DS PR feedback to end up with an ideal general implementation and shipping fast on the product side can be at odds at times

In my opinion, we shouldn't merge this current implementation to the DS if what we want long term is a split button. We should get to the implementation we want then include that in an upcoming DS release. Both because semver means that we're committing to not breaking the interface to the component until the next major version, but also because when we announce it devs are going to get used to and expect what we announce.

If shipping fast is a priority on the product side to the point where the ticket can't wait on this PR, it doesn't have to though. This implementation of DrawerFooter could be included in rails-server in the design_system_components folder and used until the DS version has been finalized and rolled out. At that point you'd just swap in the DS version.

[gabescarbrough]

That said, this is most of the way there so either way I don't think this PR is far off of the ideal general implementation. The sticking point is really the split button.

[eric2523]

Hmm i think im gonna bring this into rails-server to avoid this from spiraling too much. One of my concerns is that passing the split prop to the DropdownToggle will require some style changes and potentially changes how were encapsulating that component atm since you'll still need to put a Button component next to the split toggle. We could keep it within DrawerFooter but also mainly bc there is a big time constraint here

<Button variant="success">Split Button</Button>
      <Dropdown.Toggle split variant="success" id="dropdown-split-basic" />

[gabescarbrough]

Suggested change

	If additional primary actions are required, you can utilize the `primaryAdditionalActions` prop. This will render the primary action as a dropdown toggle.
	If additional primary actions are required, you can utilize the `additionalPrimaryActions` prop. This will render the primary action as a dropdown toggle.

[gabescarbrough]

@eric2523 my intention was that when additionalPrimaryActions is passed then the primary action should be a split button, not a dropdown. I think having a typical dropdown for a primary action is confusing. I don't think I've ever seen that in an app before. Non-split dropdowns are better suited to menus of actions as opposed to primary actions.

@kyleshike if the primary action is a split button then onPrimaryAction is what is called when you click the primary action, and the additionalPrimaryActions onClick functions would be called for the additional actions.

Using this random split button example I found on NN/g, clicking "Insert" would call onPrimaryAction. Clicking the downward arrow then clicking "Insert above" would call the onClick associating with that action.

As Jason pointed out React Bootstrap's Dropdown component supports split buttons so ours (which wraps it) should be able to as well: https://react-bootstrap.netlify.app/docs/components/dropdowns/#split-button-dropdowns

My preference would be to add a SplitButton component so we could use it elsewhere easily as well. I could see an argument for supporting split buttons as part of Button or Dropdown but it's probably more discoverable if it is its own thing. Also the props could be neater.