Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(Form): add inverted prop #1218

Merged
merged 1 commit into from
Jan 24, 2017

Conversation

vageeshb
Copy link
Contributor

@vageeshb vageeshb commented Jan 24, 2017


const FormExampleInverted = () => (
<Segment inverted={true}>
<Form inverted={true}>
Copy link
Member

Choose a reason for hiding this comment

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

Boolean props are true by default, let's go with inverted over inverted={true} 👍

@@ -9,6 +9,11 @@ const FormFormVariationsExamples = () => (
description='A form can vary in size'
examplePath='collections/Form/Variations/FormExampleSize'
/>
<ComponentExample
title='Inverted'
description='A form on a dark background may have to invert its color scheme'
Copy link
Member

Choose a reason for hiding this comment

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

Period after description please, we're currently trying to standardize this.

@@ -258,11 +261,13 @@ export default class Form extends Component {
success,
warning,
widths,
inverted,
Copy link
Member

Choose a reason for hiding this comment

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

Please alphabetically sort the destructured props.

@levithomason
Copy link
Member

Thanks much for the PR! This looks great, made a couple minor comments for you.

@levithomason levithomason changed the title feat(Form): add prop for inverted classname feat(Form): add inverted prop Jan 24, 2017
@layershifter
Copy link
Member

Please, don't forget about typings.

@codecov-io
Copy link

codecov-io commented Jan 24, 2017

Current coverage is 95.89% (diff: 100%)

Merging #1218 into master will increase coverage by <.01%

@@             master      #1218   diff @@
==========================================
  Files           879        880     +1   
  Lines          4897       4899     +2   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           4696       4698     +2   
  Misses          201        201          
  Partials          0          0          

Powered by Codecov. Last update 7b5128c...f775f0c

@vageeshb
Copy link
Contributor Author

@levithomason Updated with feedback
@layershifter Haven't worked with TS, added typings. Please verify.

<Form inverted>
<Form.Group widths='equal'>
<Form.Field label='First name' control='input' placeholder='First name' />
<Form.Field label='Last name' control='input' placeholder='Last name' />
Copy link
Member

Choose a reason for hiding this comment

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

Apologies that I missed this on the first pass. Let's use the shorthand <Form.Input /> for brevity. I like to keep the doc examples as slim as possible:

-<Form.Field label='First name' control='input' placeholder='First name' />
+<Form.Input label='First name' placeholder='First name' />

@@ -37,6 +37,7 @@ describe('Form', () => {
common.propKeyOnlyToClassName(Form, 'reply')
common.propKeyOnlyToClassName(Form, 'success')
common.propKeyOnlyToClassName(Form, 'warning')
common.propKeyOnlyToClassName(Form, 'inverted')
Copy link
Member

Choose a reason for hiding this comment

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

Nit: alphabetical by prop name please. I realize many are out of order, however, there is a current effort to update these.

@@ -263,6 +267,7 @@ export default class Form extends Component {
const classes = cx(
'ui',
size,
useKeyOnly(inverted, 'inverted'),
Copy link
Member

Choose a reason for hiding this comment

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

Nit: alphabetical by prop name please

@levithomason
Copy link
Member

Thanks for the extra fixes!

@levithomason levithomason merged commit 8d65427 into Semantic-Org:master Jan 24, 2017
@levithomason
Copy link
Member

Released in semantic-ui-react@0.64.4.

DomiR added a commit to DomiR/Semantic-UI-React that referenced this pull request Jan 26, 2017
* Semantic-Org/master: (111 commits)
  fix(docs): fix usage of arrow function (Semantic-Org#1228)
  fix(Icon): Incorrect definition in typings (Semantic-Org#1225)
  fix(Button): fix `tabIndex` in typings (Semantic-Org#1224)
  fix(typings): add item prop to the DropdownProps (Semantic-Org#1223)
  docs(examples): add missing `key` props (Semantic-Org#1220)
  docs(changelog): update changelog [ci skip]
  0.64.4
  feat(Form): add `inverted` prop (Semantic-Org#1218)
  fix(ComponentExample): use explicit babel presets (Semantic-Org#1219)
  style(Button): update typings and propTypes usage (Semantic-Org#1216)
  style(Embed): update typings and propTypes usage (Semantic-Org#1217)
  chore(package): support for jsnext:main (Semantic-Org#1210)
  style(Step): update typings, tests and propTypes usage (Semantic-Org#1203)
  fix(Portal): do not take focus after first render (Semantic-Org#1215)
  style(Table|mixed): update typings, tests and propTypes usage (Semantic-Org#1200)
  fix(Dropdown): use `item` instead of `as={Menu.Item}` (Semantic-Org#659)
  fix(Dropdown): respect `closeOnBlur={false}` (Semantic-Org#1148)
  style(Breadcrumb): update typings and propTypes usage (Semantic-Org#1209)
  style(Dimmer): update typings (Semantic-Org#1208)
  fix(Divider|FormInput): fix the broken typings (Semantic-Org#1179)
  ...
harel pushed a commit to harel/Semantic-UI-React that referenced this pull request Feb 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants