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

Allows material BoolFields to be checkboxes or toggles #189

Merged
merged 3 commits into from
Feb 10, 2017

Conversation

dpankros
Copy link
Contributor

@dpankros dpankros commented Feb 10, 2017

This PR adds an appearance property to BoolField that allows rendering as a Toggle if appearance set to 'toggle'. Default or any other value is 'checkbox' (present behavior) so as to not break any existing apps.
That is, <BoolField name='field' /> still works as it previously did by rendering a checkbox, but now you can alternatively use toggles: <BoolField name='field' appearance='toggle' />

Updated combined tests for the above (x35)

NOTE: I did not update any package version numbers.

…if appearance set to 'toggle'. Default or any other value is 'checkbox' (present behavior)

Updated combined tests for the above
Copy link
Contributor

@radekmie radekmie left a comment

Choose a reason for hiding this comment

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

All of them should be handled by ESLint, but it's not configured yet.

@@ -7,6 +7,7 @@ import {spy} from 'sinon';
import {stub} from 'sinon';

import MaterialCheckbox from 'material-ui/Checkbox';
import MaterialToggle from 'material-ui/Toggle';
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep them sorted.

@@ -1,4 +1,5 @@
import Checkbox from 'material-ui/Checkbox';
import Toggle from 'material-ui/Toggle';
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep imports sorted.

@@ -11,18 +12,28 @@ const Bool = ({
name,
onChange,
value,
appearance = 'checkbox',
Copy link
Contributor

Choose a reason for hiding this comment

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

Props also should be sorted.

name={name}
onCheck={(event, value) => disabled || onChange(value)}
ref={inputRef}{...filterDOMProps(props)}
/>);
Copy link
Contributor

Choose a reason for hiding this comment

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

Two things here:

  • keep props sorted
  • ternary should be formatted like this:
appearance === 'toggle' ? (
    <Toggle
        ...
    />
) : (
    <Checkbox
        ...
    />
)

@radekmie
Copy link
Contributor

Great! And I'm really glad to see, that you've written tests!

…mbined.js

Update: Formatting of ternary expression in BoolField.js
@dpankros
Copy link
Contributor Author

Updated.
Sorry about the non-conforming code. I didn't see any coding standards doc and these were not caught by your lint spec. The ternary is actually in conflict with an internal standard that we have so my JS IDE wouldn't even let me do it.

@radekmie
Copy link
Contributor

Yeah, I have to update ESLint finally... Thanks!

@radekmie radekmie merged commit 10ef9ba into vazco:master Feb 10, 2017
@radekmie radekmie added the Type: Feature New features and feature requests label Feb 10, 2017
@Monteth Monteth added this to Closed in Open Source (migrated) Jun 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New features and feature requests
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants