feat(Button): added ButtonGroup component by mnajdova · Pull Request #179 · microsoft/fluent-ui-react

mnajdova · 2018-09-04T12:26:25Z

ButtonGroup

The ButtonGroup component, as part of the Button component, will be used to organize multiple buttons to apear together.

TODO

API Proposal

as?: any

children?: ReactChildren

circular?: boolean

This property is used for creating group of circular buttons

<Button.Group
    circular
    buttons={[
      { icon: 'book', type: 'primary' },
      { icon: 'coffee' },
      { icon: 'play', type: 'primary' },
    ]}
  />

<div class="ui-buttons">
  <button class="ui-button ..." aria-disabled="false">
    <span class="ui-icon ..." aria-hidden="true"></span>
  </button>
  <button class="ui-button ..." aria-disabled="false">
    <span class="ui-icon ..." aria-hidden="true"></span>
  </button>
  <button class="ui-button ..." aria-disabled="false">
    <span class="ui-icon ..." aria-hidden="true"></span>
  </button>
</div>

className?: string

content?: React.ReactNode

buttons?: ItemShorthand[]

Shorthand for setting the buttons that should appear in the group.

<Button.Group buttons={[{ icon: 'book', iconOnly: true }, { icon: 'coffee', iconOnly: true }, { icon: 'play', iconOnly: true }]} />

<div class="ui-buttons">
  <button class="ui-button ..." aria-disabled="false">
    <span class="ui-icon ..." aria-hidden="true"></span>
  </button>
  <button class="ui-button ..." aria-disabled="false">
    <span class="ui-icon ..." aria-hidden="true"></span>
  </button>
  <button class="ui-button..." aria-disabled="false">
    <span class="ui-icon ..." aria-hidden="true"></span>
  </button>
</div>

styles?: IComponentPartStylesInput

variables?: ComponentVariablesInput

Question

Do we have mechanism for testing collectionShorthand (list of itemShorthand). If not, should I create one and use for testing the buttons inside the ButtonGroup component?

codecov · 2018-09-04T12:27:42Z

Codecov Report

Merging #179 into master will increase coverage by 0.6%.
The diff coverage is 84.21%.

@@            Coverage Diff            @@
##           master     #179     +/-   ##
=========================================
+ Coverage   67.09%   67.69%   +0.6%     
=========================================
  Files         101      103      +2     
  Lines        1407     1461     +54     
  Branches      290      288      -2     
=========================================
+ Hits          944      989     +45     
- Misses        459      468      +9     
  Partials        4        4

Impacted Files	Coverage Δ
.../themes/teams/components/Button/buttonVariables.ts	`66.66% <ø> (ø)`	⬆️
src/components/List/List.tsx	`100% <ø> (ø)`	⬆️
src/themes/teams/components/Button/buttonStyles.ts	`11.42% <0%> (-0.34%)`	⬇️
src/components/Button/index.ts	`100% <100%> (ø)`	⬆️
src/index.ts	`100% <100%> (ø)`	⬆️
src/components/Button/Button.tsx	`95.55% <100%> (+0.31%)`	⬆️
src/themes/teams/componentVariables.ts	`100% <100%> (ø)`	⬆️
src/themes/teams/componentStyles.ts	`100% <100%> (ø)`	⬆️
...hemes/teams/components/Button/buttonGroupStyles.ts	`30% <30%> (ø)`
src/components/Button/ButtonGroup.tsx	`97.29% <97.29%> (ø)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5277076...9453d62. Read the comment docs.

# Conflicts: # CHANGELOG.md

layershifter · 2018-09-04T18:52:41Z

@mnajdova we don't have common tests for collections in SUIR, so I think that Stardust doesn't have them, too.

mnajdova · 2018-09-05T10:21:10Z

Got it, thanks @layershifter, will add one common test for the collectionShorthand.

-updated ButtonGroup-test.tsx to confirm to the collection shorthand test

kuzhelov · 2018-09-05T14:19:51Z

test/specs/commonTests/implementsCollectionShorthandProp.tsx

+        expect(Component.propTypes[shorthandPropertyName]).toBeTruthy()
+      })
+
+      test(`array of string values are handled as ${


grammar nit: 'as' -> 'is' :)

would also recommend to consider test renaming, to better indicate what is going on, something like 'array of string values is spread as ...'

Agreed, it is better explained that way!

kuzhelov · 2018-09-05T14:22:12Z

test/specs/commonTests/implementsCollectionShorthandProp.tsx

+        const props = { [shorthandPropertyName]: shorthandValue }
+        const wrapper = mount(<Component {...props} />)
+
+        const shorthandComponentProps = wrapper.find(ShorthandComponent.displayName)


seems that name of the variable is not correct - due to wrapper.find(ShorthandComponent.displayName) returns a nodes wrapper, not props

Thanks for the catch! I changed it in the other test and forgot about this one.. :)

test/specs/commonTests/implementsCollectionShorthandProp.tsx

kuzhelov · 2018-09-05T14:29:46Z

test/specs/commonTests/implementsCollectionShorthandProp.tsx

+          _.first(shorthandValue),
+        ).every(
+          propertyName =>
+            propertyName === 'key'


can be simplified

propertyName => propertyName === 'key' || _.first(shorthandValue)[propertyName] === shorthandComponents.first().prop(propertyName)

kuzhelov · 2018-09-05T14:30:15Z

test/specs/components/Button/ButtonGroup-test.tsx


 describe('ButtonGroup', () => {
  isConformant(ButtonGroup)
+  buttonGroupImplementsShorthandProp('buttons', Button)


kuzhelov · 2018-09-05T14:33:06Z

src/index.ts

 export { default as Button } from './components/Button'
+export { ButtonGroup } from './components/Button'
 export { default as Chat } from './components/Chat'
 export { default as ChatMessage } from './components/Chat'


seems that we have a problem here - as Chat is exported as ChatMessage. Could you, please, fix this? :)

kuzhelov · 2018-09-05T14:36:01Z

src/themes/teams/components/Button/buttonVariables.ts

  height: string
  minWidth: string
  maxWidth: string
+  defaultBorderRadius: string


are we using any different other than default one for Button? If not, we should probably consider to rename it to something like just borderRadius - what do you think?

Yes, we have circularRadius as well, that is why this is named like that.

kuzhelov · 2018-09-05T14:41:30Z

src/components/Button/ButtonGroup.tsx

+class ButtonGroup extends UIComponent<Extendable<IButtonGroupProps>, any> {
+  public static displayName = 'ButtonGroup'
+
+  public static className = 'ui-button__buttons'


seems that BEM conventions are violated here - this reads as 'buttons' element inside 'ui-button' - but I would rather expect something simpler like 'ui-buttons' or 'ui-button-group'

We have utility function for generating the className inside getComponentInfo.ts:

info.componentClassName = (info.isChild
? `ui-${info.parentDisplayName}__${info.subcomponentName.replace(
/Group$/,
`${info.parentDisplayName}s`,
)}`
: `ui-${info.displayName}`
).toLowerCase()

I was just following it in order to make the component conformant. Do you think we should change this logic?

the logic of this function seems correct to me - the problem is that for info.parentDisplayName 'button' is used for the case of ButtonGroup, and this is semantically incorrect

In the end, we decided that the components that contain Group inside the name should result into the main components class with appended 's' in the end (For the ButtonGroup it should be ui-buttons). We are doing this because we already had some logic for the Group components, and we can still keep the group components together with the main components that they represent.

src/components/Button/ButtonGroup.tsx

kuzhelov · 2018-09-05T14:49:54Z

src/components/Button/ButtonGroup.tsx

+    )
+  }
+
+  getStyleForButtonIndex = (styles, buttons, idx) => {


it seems that logic of this function relies (semantically) only on isFirst and isLast flags - for the sake of making its interface lighter and more explicit, could we introduce these two instead of buttons and idx?

kuzhelov · 2018-09-05T14:53:31Z

src/components/Button/ButtonGroup.tsx

+          Button.create(button, {
+            defaultProps: {
+              circular,
+              styles: { root: this.getStyleForButtonIndex(styles, buttons, idx) },


this showcases the problem we currently have for variables as well (#162) - with current implementation approach we will have the same negative consequences for styles as the ones described there for variables. Lets consider to address this moment later

kuzhelov

please, take a look on comments before merging - nothing critical, though. Awesome job with introducing implementsCollectionShorthandProp 👍

-addressed comments on the PR

# Conflicts: # CHANGELOG.md # test/specs/components/Menu/Menu-test.tsx

-changed defaultBorderRadius to borderRadius in the button's variables

-added ButtonGroup component

f63a600

mnajdova requested review from dvdzkwsk, kuzhelov, levithomason and miroslavstastny as code owners September 4, 2018 12:26

mnajdova added the 🚀 ready for review label Sep 4, 2018

-updated CHANGELOG

105cbc9

mnajdova added the RFC label Sep 4, 2018

manajdov added 2 commits September 4, 2018 17:51

Merge branch 'master' into feat/button-group

7112472

# Conflicts: # CHANGELOG.md

-added iconOnly in the buttons group example

abc813a

-added common test for collection shorthand

e27e3c9

-updated ButtonGroup-test.tsx to confirm to the collection shorthand test

kuzhelov reviewed Sep 5, 2018

View reviewed changes

test/specs/commonTests/implementsCollectionShorthandProp.tsx Show resolved Hide resolved

kuzhelov reviewed Sep 5, 2018

View reviewed changes

src/components/Button/ButtonGroup.tsx Show resolved Hide resolved

kuzhelov reviewed Sep 5, 2018

View reviewed changes

kuzhelov approved these changes Sep 5, 2018

View reviewed changes

kuzhelov added ready for merge and removed 🚀 ready for review labels Sep 5, 2018

manajdov added 2 commits September 6, 2018 11:54

-added collection shorthand tests for the List and Menu components

7320243

-addressed comments on the PR

Merge branch 'master' into feat/button-group

be1e838

# Conflicts: # CHANGELOG.md # test/specs/components/Menu/Menu-test.tsx

manajdov added 3 commits September 6, 2018 12:01

-resolved merge conflicts

526601e

-fixed generation of the className in the groups component

75ecf4f

-changed defaultBorderRadius to borderRadius in the button's variables

Merge branch 'master' into feat/button-group

9453d62

mnajdova merged commit 4697975 into master Sep 6, 2018

levithomason deleted the feat/button-group branch October 29, 2019 22:30

Conversation

mnajdova commented Sep 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ButtonGroup

TODO

API Proposal

as?: any

children?: ReactChildren

circular?: boolean

className?: string

content?: React.ReactNode

buttons?: ItemShorthand[]

styles?: IComponentPartStylesInput

variables?: ComponentVariablesInput

Question

Uh oh!

codecov bot commented Sep 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

layershifter commented Sep 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mnajdova commented Sep 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kuzhelov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mnajdova commented Sep 4, 2018 •

edited

Loading

codecov bot commented Sep 4, 2018 •

edited

Loading

layershifter commented Sep 4, 2018 •

edited

Loading

kuzhelov Sep 5, 2018 •

edited

Loading

kuzhelov Sep 5, 2018 •

edited

Loading

kuzhelov Sep 5, 2018 •

edited

Loading