Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

feat: SplitButton component#1798

Merged
silviuaavram merged 53 commits intomasterfrom
feat/split-button-component
Sep 12, 2019
Merged

feat: SplitButton component#1798
silviuaavram merged 53 commits intomasterfrom
feat/split-button-component

Conversation

@silviuaavram
Copy link
Collaborator

@silviuaavram silviuaavram commented Aug 15, 2019

Main action should be executed by onMainButtonClick. (should be named onButtonClick?)

Other actions should be executed by onMenuItemClick. Or by having onClick on each Menu Item.

Shorthands for both button and toggleButton.

Added open by ArrowDown+Alt and close by ArrowUp+Alt.

ToDo as future task:

  • create small variation.
  • CSS according to the specs.

@vercel vercel bot temporarily deployed to staging September 10, 2019 09:22 Inactive
})

return (
<ElementType className={classes.root} {...accessibility.attributes.root} {...unhandledProps}>
Copy link
Contributor

Choose a reason for hiding this comment

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

does this mean people can use onClick and it will be invoked for both click on the button as well as click on the toggle?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, for the whole container actually. And I can't obviously what is expected

handleMenuButtonOverrides = (predefinedProps: MenuButtonProps) => ({
onMenuItemClick: (e: React.SyntheticEvent, menuItemProps: MenuItemProps) => {
this.setState({ open: false })
_.invoke(this.props, 'onOpenChange', e, { open: false, ...menuItemProps })
Copy link
Contributor

Choose a reason for hiding this comment

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

I vote for sending this.props back to the user. popup here is internal, no need to expose it. what do you think @layershifter?

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, similarly to what we have in popup:

{ ...this.props, ...{ open: false }

onOpenChange: (e: React.SyntheticEvent, popupProps: PopupProps) => {
e.stopPropagation()
this.setState({ open: popupProps.open })
_.invoke(this.props, 'onOpenChange', e, popupProps)
Copy link
Contributor

Choose a reason for hiding this comment

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

{ ...this.props, ...{ open: popupProps.open }

CHANGELOG.md Outdated
- Fix broken code editor in some doc site examples and improve error experience @levithomason ([#1906](https://github.com/stardust-ui/react/pull/1906))

### Features
- Add the `SplitButton` component @silviuavram ([#1789](https://github.com/stardust-ui/react/pull/1798))
Copy link
Member

Choose a reason for hiding this comment

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

Please move it upper, Features section has been already created


const SplitButtonExampleIconShorthand = () => <SplitButton menu={items} button={items[0]} />

export default SplitButtonExampleIconShorthand
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export default SplitButtonExampleIconShorthand
export default SplitButtonIconAndContentExampleShorthand

Oops, filename is not match

import * as React from 'react'
import { SplitButton } from '@stardust-ui/react'

const SplitButtonExampleDisabledShorthand = () => <SplitButton disabled button="New conversation" />
Copy link
Member

Choose a reason for hiding this comment

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

I understand that items are not required for this example, but can we add them?

  <SplitButton
    disabled
    menu={[
      { key: 'group', content: 'New group message' },
      { key: 'channel', content: 'New channel message' },
    ]}
    button="New conversation"
  />

<ExampleSection title="Usage">
<ComponentExample
title="Main option change"
description="A split button can have its main option changed based on last selection."
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
description="A split button can have its main option changed based on last selection."
description="A SplitButton can have its main option changed based on last selection."

nit: to be consistent between sections

* @param {SyntheticEvent} event - React's original SyntheticEvent.
* @param {object} data - All props and proposed value.
*/
onOpenChange?: ComponentEventHandler<PopupProps>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
onOpenChange?: ComponentEventHandler<PopupProps>
onOpenChange?: ComponentEventHandler<SplitButtonProps>

As I see in latest change it should be SplitButtonProps

}

class SplitButton extends AutoControlledComponent<WithAsProp<SplitButtonProps>, SplitButtonState> {
static create: Function
Copy link
Member

Choose a reason for hiding this comment

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

This thing is typed now

Suggested change
static create: Function
static create: ShorthandFactory<SplitButton>

handleMenuButtonOverrides = (predefinedProps: MenuButtonProps) => ({
onMenuItemClick: (e: React.SyntheticEvent, menuItemProps: MenuItemProps) => {
this.setState({ open: false })
_.invoke(this.props, 'onOpenChange', e, { open: false, ...this.props })
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_.invoke(this.props, 'onOpenChange', e, { open: false, ...this.props })
_.invoke(this.props, 'onOpenChange', e, { ...this.props, open: false })

Order should be change otherwise a value from props will win

onOpenChange: (e: React.SyntheticEvent, popupProps: PopupProps) => {
e.stopPropagation()
this.setState({ open: popupProps.open })
_.invoke(this.props, 'onOpenChange', e, { open: popupProps.open, ...this.props })
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_.invoke(this.props, 'onOpenChange', e, { open: popupProps.open, ...this.props })
_.invoke(this.props, 'onOpenChange', e, { ...this.props, open: popupProps.open })

})

return (
<ElementType className={classes.root} {...accessibility.attributes.root} {...unhandledProps}>
Copy link
Member

Choose a reason for hiding this comment

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

Yep, for the whole container actually. And I can't obviously what is expected


this.setState(state => {
const open = !state.open
_.invoke(this.props, 'onOpenChange', e, { open, ...this.props })
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
_.invoke(this.props, 'onOpenChange', e, { open, ...this.props })
_.invoke(this.props, 'onOpenChange', e, { ...this.props, open })

@silviuaavram silviuaavram merged commit e7a59c0 into master Sep 12, 2019
@silviuaavram silviuaavram deleted the feat/split-button-component branch September 12, 2019 12:08
layershifter pushed a commit that referenced this pull request Sep 13, 2019
* first implementation

* shorthand on slots

* fix the click handlers

* addressed comments from PR

* prevent default behavior

* address code review with ref

* moved it as a separate example

* changed the example content

* replaced specification of behaviour

* address more code review

* add behavior in tests

* moved behavior

* add styles

* focus button on select

* some minor improvements

* updated styles

* keep the Tab key handler

* refactored buttons

* updates on styles and toggle close

* addressed code review

* fix behavior test

* made component conformant

* defaultProp toggleButton

* changelog

* add more unit tests

* add more tests

* fix focus style on toggle button

* more examples

* changed the padding according to specs

* code review integration

* use focus-visible in styles

* revert style slot rename

* add best practices

* fix variable name

* update changelog

* call onOpenChange with this.props

* change onOpenChange prop type

* changelog update

* integrate code review

(cherry picked from commit e7a59c0)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants