feat(Button): support all features, update to v1 API #295

jhchill666 · 2016-06-27T10:12:28Z

Fixes #43
Fixes #46

jhchill666 · 2016-06-27T10:15:37Z

src/elements/Button/ButtonGroup.js

+  vertical: PropTypes.bool,
+
+  /** Groups can have their widths divided evenly */
+  widths: PropTypes.oneOf(ButtonGroup._meta.props.widths),


Wasn't sure what this prop should be named???

I think the real intention here is to have buttons be equal width. In the FormFields we have evenlyDivided which adds the number-word class to the group (ie five fields) by counting the field children. Evenly divided is the name of that section in the SUI docs.

I could see going with equalWidth here and counting the button children. It is also consistent with the grid class equal width which scales column widths.

In order to not tie the user's hands should they want odd sized button groups, we could offer both equalWidth and widths. Or, just use something like widths='equal'.

It is worth mentioning here that we also want to do away with the word number classes in this library. Or at least offer a side by side alternative. You can see this in the Grid Column where we use width={3} instead of width='three'.

We could do this easily by extending the sui.widths to include the numbers. Then, extend the numberToWord util to include a map of words to words. Finally, always use utils/numberToWord.js on the width prop values.

This way, users can use either numbers or words to define widths wherever they appear.

See here for the width API decision https://github.com/TechnologyAdvice/stardust/issues/183#issuecomment-232728080

codecov-io · 2016-06-27T10:31:17Z

Current coverage is 98.68% (diff: 100%)

Merging #295 into master will increase coverage by 0.04%

@@             master       #295   diff @@
==========================================
  Files           101        103     +2   
  Lines          1467       1516    +49   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1447       1496    +49   
  Misses           20         20          
  Partials          0          0

Powered by Codecov. Last update ad330a3...0f8010c

levithomason · 2016-06-27T20:51:38Z

docs/app/Examples/elements/Button/Groups/ButtonColoredButtonsExample.js

+        </Button>
+        <Button>
+          Three
+        </Button>


Can we collapse these solely to save doc site example space?

<Button>One</Button> <Button>Two</Button> <Button>Three</Button>

Sure, we're already expanded I previous examples and conscious about changing you guys stuff!

Sent from my iPhone

On 27 Jun 2016, at 21:51, Levi Thomason [email protected] wrote:

In docs/app/Examples/elements/Button/Groups/ButtonColoredButtonsExample.js:

+import React, { Component } from 'react'
+import { Button } from 'stardust'
+
+export default class ButtonColoredButtonsExample extends Component {

render() {

return (

<Button.Group color='blue'>

<Button>

One

</Button>

<Button>

Two

</Button>

<Button>

Three

</Button>
Can we collapse these solely to save doc site example space?

One
Two
Three
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

At this point, I'd like to see more of your ideas as they have been great. 😉

levithomason · 2016-06-27T21:14:41Z

As part of this PR let's take on:

Add toggle button functionality #43 Toggle functionality
Create attached button element #46 Nested attached button issue

levithomason · 2016-06-27T21:24:28Z

src/elements/Button/Button.js

+  } = props
+
+  const classes = cx('sd-button', 'ui',
+    icon && 'icon',


We have a tricky bit here to deal with. The presence of an icon will not always necessitate the icon class. The icon class collapses padding to work with individual icon buttons:

I'm not sure of a good solution here yet. I'll go through this the examples and your suggestion about Button.Left, etc in more detail later.

levithomason · 2016-06-28T03:02:37Z

src/elements/Button/ButtonGroup.js

+    basic, className, children, color, icon, size, vertical, widths,
+  } = props
+
+  const classes = cx('sd-button-group', 'ui',


Heads up, I've finished removing all sd-* classes, see #301. Rebase to master to get the update. The test for the sd-* class has been inverted to fail if any sd- class exists.

Let's remove all sd-* classes from all PRs. 👍

levithomason · 2016-06-28T08:46:25Z

It's pretty late, I hope this isn't full of errors!

Button API Proposal

I was taking a look at Material-UI Buttons and I noticed they separated FlatButton, IconButton, RaisedButton, and FloatingActionButton.

This got me thinking if we could benefit from splitting up AnimatedButton, LabeledButton, IconButton, and LabeledIconButton components. Each of these varies pretty wildly in markup.

It is worth looking at. After writing these examples out and grokking SUI "labeled" as it related to Labels and Icons, I still come back to the approach of a labeled='left' prop. I think this makes the most sense now. Let me know what you think.

Animated

The animated prop feels OK to me here. The .Content sub component follows the pattern of other components. They just feel weird together here. Not so sure on this one.

<Button animated>
  <Button.Content visible>Next</Button.Content>
  <Button.Content hidden>
    <Icon name='right arrow' />
  </Button.Content>
</Button>
<Button animated='vertical'>
  <Button.Content hidden>Shop</Button.Content>
  <Button.Content visible>
    <Icon name='shop' />
  </Button.Content>
</Button>
<Button animated='fade'>
  <Button.Content visible>Sign-up for a Pro account</Button.Content>
  <Button.Content hidden>
    $12.99 a month
  </Button.Content>
</Button>

<div class="ui animated button" tabindex="0">
  <div class="visible content">Next</div>
  <div class="hidden content">
    <i class="right arrow icon"></i>
  </div>
</div>
<div class="ui vertical animated button" tabindex="0">
  <div class="hidden content">Shop</div>
  <div class="visible content">
    <i class="shop icon"></i>
  </div>
</div>
<div class="ui animated fade button" tabindex="0">
  <div class="visible content">Sign-up for a Pro account</div>
  <div class="hidden content">
    $12.99 a month
  </div>
</div>

Labeled

In SUI, the "labeled" part of the button is either a Label or an Icon, never both. If both are present, the Label is put in the labeled position and the Icon is left inside the button.

Since the labels inside a button can easily be complex, I think it makes sense to allow the Label as a child. We can also take in a label prop for basic label text like we do with icon.

<Button labeled icon='heart'>
  <Label basic>2,048</Label>
  Like
</Button>
<Button labeled='left' icon='heart'>
  <Label basic pointing='right'>2,048</Label>
  Like
</Button>
// basic labeled buttons could be achieved with a `label` prop
<Button labeled='left' icon='fork' label='1048 />

<div class="ui labeled button" tabindex="0">
  <div class="ui button">
    <i class="heart icon"></i> Like
  </div>
  <a class="ui basic label">
    2,048
  </a>
</div>
<div class="ui left labeled button" tabindex="0">
  <a class="ui basic right pointing label">
    2,048
  </a>
  <div class="ui button">
    <i class="heart icon"></i> Like
  </div>
</div>
<div class="ui left labeled button" tabindex="0">
  <a class="ui basic label">
    1,048
  </a>
  <div class="ui icon button">
    <i class="fork icon"></i>
  </div>
</div>

<Button labeled color='red' icon='heart'>
  <Label basic color='red' pointing='left'>1,048</Label>
  Like
</Button>
<Button labeled basic color='blue' icon='fork'>
  <Label basic color='blue' pointing='left'>1,048</Label>
  Forks
</Button>

<div class="ui labeled button" tabindex="0">
  <div class="ui red button">
    <i class="heart icon"></i> Like
  </div>
  <a class="ui basic red left pointing label">
    1,048
  </a>
</div>
<div class="ui labeled button" tabindex="0">
  <div class="ui basic blue button">
    <i class="fork icon"></i> Forks
  </div>
  <a class="ui basic left pointing blue label">
    1,048
  </a>
</div>

Icon

An Icon button is a button that only has an icon. This is what adds the icon class. Otherwise, it is just a button with an Icon inside of it, which may also have text beside it.

<Button icon='cloud' />
// or
<Button>
  <Icon name='cloud' />
</Button>

<button class="ui icon button">
  <i class="cloud icon"></i>
</button>

Labeled Icon

Per SUI, the labeled prop applies to the Icon if there is no Label also. Buttons with labeled and icon props receive an icon class as well.

<Button labeled='left' icon='pause'>
  Pause
</Button>
<Button labeled='right' icon='pause'>
  Pause
</Button>

<button class="ui labeled icon button">
  <i class="pause icon"></i>
  Pause
</button>
<button class="ui right labeled icon button">
  <i class="right arrow icon"></i>
  Next
</button>

Basic

I throw this one in just to show that a Button with text and an Icon does not get an icon class. The icon class is assigned if it has only an Icon, or has an Icon and is labeled.

<Button basic icon='user'>
  Add Friend
</Button>
// or
<Button basic>
  <Icon name='user' />
  Add Friend
</Button>

<button class="ui basic button">
  <i class="icon user"></i>
  Add Friend
</button>

levithomason · 2016-07-05T21:29:02Z

docs/app/Examples/elements/Button/Content/ButtonConditionalsExample.js


 export default class ButtonConditionalsExample extends Component {
  render() {
    return (
-      <Buttons>
+      <Button.Group>
        <Button>Cancel</Button>
        <div className='or' />
        <Button className='positive'>Save</Button>


We should pull positive into a prop as well.

jhchill666 · 2016-07-05T21:51:57Z

I'm going to have to go for the first option, just feels cleaner and more succinct. I'm sure I recall seeing other primary/secondary props on another component(?) is this correct? If so how would the same apply to it?

levithomason · 2016-07-05T21:56:23Z

I'm sure I recall seeing other primary/secondary props on another component(?) is this correct? If so how would the same apply to it?

Yep, the Segment for example also has emphasis. Though, in that case the options are secondary and tertiary.

Even though these are mutually exclusive attributes, you cannot have both secondary and tertiary nor primary and secondary, I think I'm with you in choosing the first option. It just doesn't seem like emphasis makes intuitive sense as an enum prop.

levithomason · 2016-07-05T22:07:12Z

src/elements/Button/Button.js

+  const rest = getUnhandledProps(Button, props)
+
+  return (
+    <button type={type} className={classes} {...rest}>


When the attached prop is used, the button component needs to be a div. This fixes #46. The CSS does not work on a button element for attached buttons.

<div class="ui top attached button" tabindex="0">Top</div> <div class="ui attached segment"> <p></p> </div> <div class="ui bottom attached button" tabindex="0">Bottom</div>

Also, the tabIndex={0} prop will need to be added so users can still tab to the button div.

Now uses div tag when !!attached && type != 'submit' as per #46

tabIndex=0 added when tag type is div

levithomason · 2016-08-09T18:34:10Z

@jamiehill hope all is well. Did you plan on picking this PRs back up or is it OK if I resume work on them?

jhchill666 · 2016-08-09T18:42:31Z

Hey Levi. Absolutely, been on hols such not much productivity of late. If you're keen to get going straight away, you take it but I'll be back on board in next few days

levithomason · 2016-08-09T18:53:52Z

Thanks for the quick reply. I'll work on other things in the interim, if I run out I'll let you know before stepping on your work.

levithomason · 2016-08-27T18:35:42Z

Per our emails, I've rebased and updated this PR to the latest master. There are some failing tests to fix with the Button prop to className.

Note all component utils are now imported from one place, ../../lib. One of those utils is getElementType. It is similar to getUnhandledProps, except instead of returning the rest of the props, it returns the Element type the component should render as.

There is one feature needed for this new util. Currently, it accepts a hash map of props to element types. Example, { attached: 'div' } will render as a div when the attached prop is present. Though, the button requires the ability to render a particular element type based on the the value of the type prop, not simply the presence of it.

I've added a todo in Button.js noting this. This PR should extend the getElementType lib function to be able to select an element type based on the value of a prop, not just the presence of the prop only.

LMK if you have any issues/questions otherwise. Cheers!

levithomason · 2016-08-27T18:35:54Z

@jamiehill ^

levithomason · 2016-09-08T14:13:42Z

I've finished the labeled button API. Just need to add tests now.

levithomason · 2016-09-09T20:46:29Z

Woop! I think she's ready. Just got done hashing the remaining APIs with our team and making updates. Hope to merge when tests pass.

jhchill666 reviewed Jun 27, 2016
View reviewed changes

levithomason reviewed Jun 27, 2016
View reviewed changes

levithomason mentioned this pull request Jun 28, 2016

Update legacy component APIs to the v1 API #269

Closed

14 tasks

levithomason reviewed Jun 28, 2016
View reviewed changes

levithomason modified the milestone: v1.0 Jun 30, 2016

levithomason mentioned this pull request Jul 2, 2016

Create attached button element #46

Closed

levithomason reviewed Jul 5, 2016
View reviewed changes

levithomason added the enhancement label Jul 5, 2016

levithomason reviewed Jul 5, 2016
View reviewed changes

dvdzkwsk mentioned this pull request Jul 12, 2016

Rename plural group components (Fields) -> Group #203

Closed

6 tasks

levithomason mentioned this pull request Jul 14, 2016

<Step /> Component #183

Closed

levithomason changed the title ~~Button Component updated to v1 API~~ Button: update to v1 API Aug 18, 2016

levithomason force-pushed the feature/button branch from 70ed52c to 1e5180c Compare August 27, 2016 18:30

levithomason force-pushed the feature/button branch 2 times, most recently from 30f0d0a to 31a62c1 Compare September 7, 2016 18:10

levithomason force-pushed the feature/button branch from bde039d to c9f5917 Compare September 9, 2016 02:39

levithomason mentioned this pull request Sep 9, 2016

fix(createFactory): do not create elements from null values #480

Merged

jhchill666 and others added 19 commits September 9, 2016 13:46

feat(Button): update to v1 API

997060c

fix(Button): fix tests and element type

58d3739

docs(Button): refactor to v1 structure

b75183e

fix(Button): disallow children with shorthand props

0f6866b

test(Confirm): fix button tests

87208e7

feat(Button): support labeled buttons

b1663f6

refactor(Button): use debug for debugging

4ea5b50

docs(Button): fix active example

64dfce1

docs(Button): fix loading example

d5a3dde

docs(Button): fix toggle example

f997ac1

feat(Button): add social colors

3c06026

feat(ButtonGroup): add attached variation

d6ede26

docs(ButtonGroup): add attached variation

751b763

feat(ButtonGroup): begin group variation support

e91398f

fix(Button): fix icon/label propTypes

9407325

test(Button): add common labeled test

2f973de

feat(Button): finish docs and tests

b0a9558

fix(ButtonGroupBasicColoredExample): happy linter

1750249

feat(Button): add "animated" and "or"

1c47dae

levithomason force-pushed the feature/button branch from da495ab to 1c47dae Compare September 9, 2016 20:50

refactor(Button): complete button TODOs

0f8010c

levithomason merged commit 929a878 into Semantic-Org:master Sep 9, 2016

levithomason deleted the feature/button branch September 9, 2016 21:16

levithomason changed the title ~~Button: update to v1 API~~ feat(Button): support all features, update to v1 API Sep 9, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Button): support all features, update to v1 API #295

feat(Button): support all features, update to v1 API #295

jhchill666 commented Jun 27, 2016 •

edited by levithomason

Loading

jhchill666 Jun 27, 2016

levithomason Jun 27, 2016

levithomason Jun 27, 2016

levithomason Jul 14, 2016

codecov-io commented Jun 27, 2016 •

edited

Loading

levithomason Jun 27, 2016

jhchill666 Jun 27, 2016

levithomason Jun 27, 2016

levithomason commented Jun 27, 2016 •

edited

Loading

levithomason Jun 27, 2016

levithomason Jun 28, 2016

levithomason commented Jun 28, 2016

levithomason Jul 5, 2016

jhchill666 commented Jul 5, 2016

levithomason commented Jul 5, 2016

levithomason Jul 5, 2016

jhchill666 Jul 6, 2016

jhchill666 Jul 6, 2016

levithomason commented Aug 9, 2016

jhchill666 commented Aug 9, 2016

levithomason commented Aug 9, 2016

levithomason commented Aug 27, 2016

levithomason commented Aug 27, 2016

levithomason commented Sep 8, 2016

levithomason commented Sep 9, 2016

feat(Button): support all features, update to v1 API #295

feat(Button): support all features, update to v1 API #295

Conversation

jhchill666 commented Jun 27, 2016 • edited by levithomason 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

codecov-io commented Jun 27, 2016 • edited Loading

Current coverage is 98.68% (diff: 100%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason commented Jun 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason commented Jun 28, 2016

Button API Proposal

Animated

Labeled

Icon

Labeled Icon

Basic

Choose a reason for hiding this comment

jhchill666 commented Jul 5, 2016

levithomason commented Jul 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason commented Aug 9, 2016

jhchill666 commented Aug 9, 2016

levithomason commented Aug 9, 2016

levithomason commented Aug 27, 2016

levithomason commented Aug 27, 2016

levithomason commented Sep 8, 2016

levithomason commented Sep 9, 2016

jhchill666 commented Jun 27, 2016 •

edited by levithomason

Loading

codecov-io commented Jun 27, 2016 •

edited

Loading

levithomason commented Jun 27, 2016 •

edited

Loading