Split Button - initial concept. Includes Docs update.

[pjfitzgibbons]

Description

Implement SplitButton component

Screen.Recording.2024-01-08.at.08.55.34.mov

Issues Resolved

Fixes #1187

Check List

• New functionality includes testing.
• New functionality has been documented.
• All tests pass
  • yarn lint
  • yarn test-unit
• Update CHANGELOG.md
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[ashwin-pc]

@pjfitzgibbons can you attach some screenshots so that we don't have to pull down the PR to confirm visual changes?

[pjfitzgibbons]

@ashwin-pc updated.

I am wondering if the hairline between Primary button and drop-down button needs to be more styled?

[pjfitzgibbons]

@ashwin-pc @canascar @shanilpa Could you please label this with "2.0.0" and "1.x" per the RELEASING.md ?

[pjfitzgibbons]

Per conversation w/ @ashwin-pc , we decided the prudent option was to except this and track an issue for the typedef conflict.

[joshuali925]

it happens because of

oui/src/components/split_button/split_button_control.tsx

Line 123 in 59edc96

{...propsForDropdownButton}

, i commented on your issue

[canascar]

@pjfitzgibbons It seems like the click action triggers two independent button animations - 1 on the clicked individual inner button and 1 on the container. Currently it appears the way this split button has been built is using two separate buttons (a Button with a label and a Button with a down arrow icon wrapped in a container that inherits button animation properties. I would opt to remove the animation all together to avoid this unwanted animation behavior - this animation works best when it's a singular button and maybe not so well as a split.

Consider, for example, the way Button groups are treated - button Y animations are eliminated:
https://oui.opensearch.org/1.3/#/navigation/button#button-groups

@kgcreative any thoughts?

[kgcreative]

Yeah,to add to this, it looks like each individual element has all the chrome of the parent (individual borders, corner radii, etc). That is firmly not in the spirit of this component.The outer border and the animation when you click the button should apply to the outer container. When you click the button, you should get the depress behavior on the whole element. When you click the drop down, there should be no animation. This is definitely a 2.13 follow-up. If I have some time I may do a couple of quick animations to illustrate the difference, but when you look at Carlos' design, the border between the buttons is a single vertical bar, not two left and right borders that come together.

[pjfitzgibbons]

I have addressed and solved the issues for both animation and hairline.

[ashwin-pc]

@joshuali925 Nice catch! @pjfitzgibbons can we fix this then. Just moving the color prop to after propsForDropdownButton should fix this.

[pjfitzgibbons]

@kgcreative @ashwin-pc @BSFishy @joshuali925 @canascar
Ready for review!

[BSFishy]

I think I addressed all of these in my review comments, but just reiterating so I don't forget:

• All of the borders are currently the primary color, making theming color not make much sense
• This is a small nit, and I can totally see how I could be wrong, but I feel like making the divider border between the primary button and the dropdown button smaller makes them less distinct and harder to distinguish. cc @kgcreative on this one
• I feel like string option display either shouldn't be colored or should have more theming. For example, coloring the highlight with the color as well would look a lot better.

[BSFishy]

Why not just set these as regular JSX attributes?

[BSFishy]

nit: capital case would be cool

[BSFishy]

Don't we have autoprefixer? @AMoo-Miki

[BSFishy]

These should be based off of $ouiSize: https://oui.opensearch.org/1.3/#/guidelines/sass

[pjfitzgibbons]

Button uses $ouiBorderRadius, so duplicated here.

[BSFishy]

In my testing, I wasn't able to activate these selectors

[BSFishy]

This color should be based off of the color of button that is selected. Otherwise other colors will have a primary border

[BSFishy]

I don't think this is needed right? The element is already div, so display should be block by default

[BSFishy]

The option click actions shouldn't occur here. They should be put on the primary button so I am able to select whichever button I want to use and perform the operation in 2 separate steps.

[ashwin-pc]

Spoke offline. This is fine since the action thats performed on click is determined by the parent component, both switching options and immediately executing the option are possible like this.

[pjfitzgibbons]

@mattsb42-aws

• The button interaction should happen in 2 distinct steps, like with the Github merge button. 1: open the dropdown and select which button you want active. 2: click the button to perform the action. Currently, these are mangled into a single step: open the dropdown and click one of the options to perform the action.

In discussion with @ashwin-pc, we decided that providing the base functionality is the preferred solution. The user, through use of item's onClick() and the SplitButton text content ({children}), the GH experience can be achieved.

Is this satisfactory?

[BSFishy]

@kgcreative and I just had a conversation going over all of these points. I'll try to summarize what we found:

• The component should support 2 different types of interactions. Both of these should be shown on the docsite, to provide a quick jumping off point for builders
  • Action on click: When I click an option in the dropdown, that action is immediately performed. This would be like what happens with log explorer Generate vs Generate and Run
  • Change on click: This is more aligned with Github's merge button, where the state of the button is changed when an option is selected, then to perform the action I must press the button.
• Borders should update given the color passed in, like with regular OuiButton. All the theme styling should be in parity with OuiButton
• Styled highlighting should happen on options. When I open the dropdown, the focus color should match the color of the button.
• The animation on hover, etc. doesn't work on Firefox. Need to dig a little more on why

[ashwin-pc]

The component should support 2 different types of interactions. This should most likely be chosen as a separate prop.

@BSFishy that was the original design that @pjfitzgibbons created. I asked him to remove it because both types of interactions could be achieved with just one flow because the parent component could anyways decide how to render the component when the options are clicked. I prefer this to using a prop to toggle between the two interaction patterns because it makes the component props cleaner and more easy to maintain. Adding an extra prop when the same functionality can be achieved with the existing props feels unnecessary imo.

Also this way its a two way door decision. If we dont like it and feel the need for that prop, we can always add it in the future in a minor release. It does not need to block the release of this component

[pjfitzgibbons]

@ashwin-pc I need you to see above and agree on the direction. You are a reviewer and if you disagree then we need to discuss further.

cc: @kgcreative @BSFishy

[kgcreative]

@ashwin-pc I need you to see above and agree on the direction. You are a reviewer and if you disagree then we need to discuss further.

cc: @kgcreative @BSFishy

This may be more of a documentation issue than a component design issue. If we don't need a prop for click + switch vs switch, then we should just show the variations as examples.

[pjfitzgibbons]

I can definitely put examples on the doc. I had that in mind, and if it's a critical go/nogo component of the feature, then I'll put it in there. Meanwhile I'm addressing the technical findings. Thanks and Kindest Regards, Peter Fitzgibbons

…

On Mon, Jan 8, 2024 at 11:39 AM Kevin Garcia ***@***.***> wrote: @ashwin-pc <https://github.com/ashwin-pc> I need you to see above and agree on the direction. You are a reviewer and if you disagree then we need to discuss further. cc: @kgcreative <https://github.com/kgcreative> @BSFishy <https://github.com/BSFishy> This may be more of a documentation issue than a component design option. If we don't need a prop for click + switch vs switch, then we should just show the variations as examples. — Reply to this email directly, view it on GitHub <#1193 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAACYNRUSOREXKCLYSDLM53YNRDPRAVCNFSM6AAAAABBDUVJOWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOBRG4YTAMZVGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[BSFishy]

Yeah, I just got aligned with Ashwin. So we want both scenarios to be documented: action on click and change on click. Here is the change on click example I was thinking of:

export default () => {
  const [selectedIndex, setSelectedIndex] = useState(0);

  const options = [
    {
      display: (
        <Fragment>
          <strong>Option one</strong>
          <OuiText disabled size="s" color="subdued">
            Has a short description giving more detail to the option.
          </OuiText>
        </Fragment>
      ),
      button: 'Option one',
      onClick: () => setSelectedIndex(0),
      onButtonClick: () => console.log('Option one clicked'),
    },
    {
      display: (
        <Fragment>
          <strong>Option two</strong>
          <OuiText size="s" color="subdued">
            Has a short description giving more detail to the option.
          </OuiText>
        </Fragment>
      ),
      button: 'Option two',
      onClick: () => setSelectedIndex(1),
      onButtonClick: () => console.log('Option two clicked'),
    },
    {
      display: 'Just some Text',
      button: 'Option three',
      onClick: () => setSelectedIndex(2),
      onButtonClick: () => console.log('Option three clicked'),
    },
  ];

  return (
    <OuiSplitButton
      options={options}
      selectedIndex={selectedIndex}
      onClick={options[selectedIndex].onButtonClick}
      hasDividers>
      {options[selectedIndex].button}
    </OuiSplitButton>
  );
};

This example somewhat works, but some of the interactions still need to be fixed.

(TLDR; we're aligned on the current approach, no more effort needed on that front. Just rounding out the edges. I'll update my review comment to reflect this)

[ashwin-pc]

Thanks for making many of the changes @pjfitzgibbons. Just a few more comments but its looks close to complete.

1. There still exists the extra space that i called out in my last review when selectedIndex is undefined. cc: @kgcreative, @canascar what do you think?
2. The key interactions are fine for the initial implementation but i feel like these can be improved. Maybe something we can target for a future release. One thing i did want to highlight for this iteration though is that we arent closing the dropdown when the user tabs out of the dropdown. Can we fix that?
3. This has been called out already but I too feel like the border color not matching the behavior of Button needs to be fixed.

[ashwin-pc]

Thanks for creating this issue :)

[ashwin-pc]

I like the tests you have added.

[ashwin-pc]

nit: Can we name this similar to textProps for Button.

Suggested change

	/**
	* Optional additional props to send to Primary Button
	*/
	propsForPrimaryButton?: OuiButtonProps;
	/**
	* Optional additional props to send to Dropdown Button
	*/
	propsForDropdownButton?: OuiButtonProps;
	/**
	* Optional additional props to send to each Option Item, when
	* it is string, rendered with OuiText wrapper
	*/
	propsForOptionItems?: OuiTextProps;
	/**
	* Optional additional props to send to Primary Button
	*/
	buttonProps?: OuiButtonProps;
	/**
	* Optional additional props to send to Dropdown Button
	*/
	dropdownProps?: OuiButtonProps;
	/**
	* Optional additional props to send to each Option Item, when
	* it is string, rendered with OuiText wrapper
	*/
	optionProps?: OuiTextProps;

[ashwin-pc]

Spoke offline. This is fine since the action thats performed on click is determined by the parent component, both switching options and immediately executing the option are possible like this.