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.

Sign up for GitHub

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

Jump to bottom

Add radio group controls for enums (#989) #1029

Merged
merged 1 commit into from
Aug 3, 2018
Merged

Add radio group controls for enums (#989) #1029

merged 1 commit into from
Aug 3, 2018

Conversation

AlexandraBuzila
Copy link
Member

@AlexandraBuzila AlexandraBuzila commented Jul 13, 2018 •
edited
Loading

Adds MaterialRadioGroupControl and vanilla RadioGroupControl that renders enums as radio groups.

selection_242

@AlexandraBuzila AlexandraBuzila force-pushed the radio-group-control branch 3 times, most recently from 1c9c733 to 38a273c Compare July 13, 2018 13:45
edgarmueller
edgarmueller suggested changes Jul 13, 2018
View reviewed changes
Copy link
Contributor

@edgarmueller edgarmueller left a comment

Choose a reason for hiding this comment

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

This looks good, but is the vanilla renderer missing?

packages/material/src/controls/MaterialRadioGroupControl.tsx Outdated
if (!visible) {
style.display = 'none';
}
const inputLabelStyle: { [x: string]: any } = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this label style?

packages/examples/src/rule.ts Outdated
@@ -41,7 +41,7 @@ export const schema = {
};

export const uischema = {
type: 'HorizontalLayout',
type: 'VerticalLayout',
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather a new, explicit example for demonstrating the radio button group and revert this change.

packages/material/src/controls/MaterialRadioGroupControl.tsx
FormLabel
} from '@material-ui/core';

export class MaterialRadioGroupControl extends Control<ControlProps, ControlState> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have a vanilla version of this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Pushed new commit containing vanilla renderer as well.

packages/material/test/renderers/MaterialRadioGroupControl.test.tsx Outdated
expect(radioButtons.length).toBe(4);
let currentlyChecked = _.filter(radioButtons, radio => radio.checked);
expect(currentlyChecked.length).toBe(1);
expect(currentlyChecked[0].value).toBe('D');
Copy link
Contributor

Choose a reason for hiding this comment

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

We tested this expectation in the test before this one. Perhaps remove the assertions here and rename the tests accordingly. This test should only whether the binding works as intended.

@edgarmueller edgarmueller added this to the 2.0.4 milestone Jul 13, 2018
@AlexandraBuzila AlexandraBuzila changed the title Add MaterialRadioGroupControl (#989) Add radio group controls for enums (#989) Jul 19, 2018
@coveralls
Copy link

coveralls commented Jul 19, 2018 •
edited
Loading

Coverage Status

Coverage decreased (-0.004%) to 89.457% when pulling 446d018 on radio-group-control into 1fc673b on master.

@eneufeld
Copy link
Member

General comment:
please clone this repo, and create feature branches in your clone and open PRs against your repo branch.

eneufeld
eneufeld requested changes Jul 27, 2018
View reviewed changes
Copy link
Member

@eneufeld eneufeld left a comment

Choose a reason for hiding this comment

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

The Material Control is ok.
But the FormLabel is much bigger then all other labels in the form. Could you look into it?
Shouldn't we display Material and Vanilla RadioGroup in the same way?
Material is showing the options as row, and vanilla as column.

packages/vanilla/src/controls/RadioGroupControl.tsx Outdated
{computeLabel(isPlainLabel(label) ? label : label.default, required)}
</label>

<form>
Copy link
Member

Choose a reason for hiding this comment

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

why wrap in a form?

Copy link
Member Author

Choose a reason for hiding this comment

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

I replaced it with a div, needed for styling.

packages/vanilla/src/controls/RadioGroupControl.tsx Outdated
{
options.map(optionValue =>
(
<div>
Copy link
Member

Choose a reason for hiding this comment

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

why wrap each input into an own div?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because we need to return an input and a label.

packages/vanilla/src/controls/RadioGroupControl.tsx
import { VanillaControlProps } from '../index';
import { addVanillaControlProps } from '../util';

export class RadioGroupControl extends Control<VanillaControlProps, ControlState> {
Copy link
Member

Choose a reason for hiding this comment

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

In my opinion this can be a field, at least in the way it is currently implemented

Copy link
Member Author

Choose a reason for hiding this comment

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

Discussed it with Edgar, we left it as a control for now.

@AlexandraBuzila
Copy link
Member Author

the FormLabel is much bigger then all other labels in the form.

It is the same in the material examples: the FormLabel is big and does not change size when a value is selected in the radio group.
https://material-ui.com/demos/selection-controls/#radio-buttons

Shouldn't we display Material and Vanilla RadioGroup in the same way?
Material is showing the options as row, and vanilla as column.

I fixed it in the new commit.

@edgarmueller edgarmueller merged commit 3210474 into master Aug 3, 2018
@sdirix sdirix deleted the radio-group-control branch December 11, 2018 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eneufeld eneufeld eneufeld requested changes

@edgarmueller edgarmueller edgarmueller approved these changes

Assignees

@edgarmueller edgarmueller

Labels
None yet
Projects
None yet
Milestone
2.0.7
Development

Successfully merging this pull request may close these issues.
4 participants
@AlexandraBuzila @coveralls @eneufeld @edgarmueller