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(core/presentation): Support inline radio buttons #7078

Conversation

christopherthielen
Copy link
Contributor

This PR changes the following things in RadioButtonInput:

  • adds support for inline radio buttons.
  • adds stringOptions prop instead of overloading options which previously took either an array of Option objects or an array of strings (I'm open to feedback on this API change)
  • switches from a Set of options to an array, so ordering can be applied.

I'd primarily like feedback from @anotherchrisberry but he's OOO so I'm tagging Erik and Alan.


This PR also reverts tweaks to global bootstrap CSS that adjust margins and padding.

before:
Screen Shot 2019-06-03 at 6 22 18 PM

after:
Screen Shot 2019-06-03 at 5 47 04 PM


Finally, this adds support for wrapping of inline checkboxes and radio buttons using a margin hack. This puts the margin between successive elements on the right of the element instead of the left. This is to support wrapping of inline checkboxes and radios.

before:
Screen Shot 2019-06-03 at 5 53 00 PM

after:
Screen Shot 2019-06-03 at 5 53 39 PM

cc: @Jammy-Louie I believe this component should allow you to drop this duplicate component:

export const CloudFoundryRadioButtonInput = (props: ICloudFoundryRadioButtonInputProps) => {
const { value, validation, inputClassName, options, ...otherProps } = props;
const className = `RadioButtonInput radio ${orEmptyString(inputClassName)} ${validationClassName(validation)}`;
const RadioButtonsElement = ({ opts }: { opts: Array<Option<string>> }) => (
<div className="horizontal">
{opts.map(option => (
<div key={option.label} className={className}>
<label className="cloud-foundry-radio-button">
<input
type="radio"
{...otherProps}
value={option.value}
checked={option.value === value}
onChange={e => {
if (props.onChange) {
props.onChange(e);
}
}}
/>
<span className="marked">
<Markdown message={option.label} />
</span>
</label>
</div>
))}
</div>
);
if (isStringArray(options)) {
return <StringsAsOptions strings={options}>{opts => <RadioButtonsElement opts={opts} />}</StringsAsOptions>;
} else {
return <RadioButtonsElement opts={options as Array<Option<string>>} />;
}
};

- Remove inline checkbox and radio button CSS tweaks (revert
margin/padding to Bootstrap 3 defaults)
- Add margin hack for inline radio/checkboxes to support wrapping to the
next line
Copy link
Member

@erikmunson erikmunson left a comment

Choose a reason for hiding this comment

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

don't feel like I have enough/any context on the motivation for splitting out stringOptions/options, so no real opinion there.

const RadioButton = ({ option }: { option: Option }) => (
<label key={option.label} className={inline ? 'radio-inline clickable' : 'inline clickable'}>
<input type="radio" {...otherProps} value={option.value as any} checked={option.value === selectedValue} />
<Markdown message={option.label} />
Copy link
Member

Choose a reason for hiding this comment

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

I realize this was already here before the diff, purely curiosity: is this intended for html sanitization/support, actual markdown support, or both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's for markdown support

Screen Shot 2019-06-04 at 9 34 18 AM

@Jammy-Louie
Copy link

When its merged I'll remove the provider specific implementation in favour of this one.

@maggieneterval
Copy link
Contributor

I can't wait to use this, looks great! 🔥

@christopherthielen christopherthielen merged commit 9bbc002 into spinnaker:master Jun 4, 2019
@christopherthielen christopherthielen deleted the radio-button-input-inline branch June 4, 2019 17:01
christopherthielen added a commit to christopherthielen/deck that referenced this pull request Jun 5, 2019
8398d77 refactor(*): make accountExtractor return an array of strings (spinnaker#7068)
e6d1586 feat(core/presentation): support JSX for validationMessage in FormFields (spinnaker#7083)
78a9f72 fix(core): Disable rewriteLinks to allow proper event handling (spinnaker#7064)
3f21c1f feat(executions): Adding redirect for execution without application (spinnaker#7076)
f89d97d feat(core/presentation): Add "success" type to ValidationMessage (spinnaker#7082)
4e89a39 refactor(core/presentation): Consolidate Checklist and ChecklistInput components (spinnaker#7077)
9bbc002 feat(core/presentation): Support inline radio buttons (spinnaker#7078)
b142789 fix(triggers): Poll on execution id instead of event id (spinnaker#7067)
6af93e3 refactor(pipeline): Pipeline stage execution details to react (spinnaker#7063)
christopherthielen added a commit that referenced this pull request Jun 5, 2019
8398d77 refactor(*): make accountExtractor return an array of strings (#7068)
e6d1586 feat(core/presentation): support JSX for validationMessage in FormFields (#7083)
78a9f72 fix(core): Disable rewriteLinks to allow proper event handling (#7064)
3f21c1f feat(executions): Adding redirect for execution without application (#7076)
f89d97d feat(core/presentation): Add "success" type to ValidationMessage (#7082)
4e89a39 refactor(core/presentation): Consolidate Checklist and ChecklistInput components (#7077)
9bbc002 feat(core/presentation): Support inline radio buttons (#7078)
b142789 fix(triggers): Poll on execution id instead of event id (#7067)
6af93e3 refactor(pipeline): Pipeline stage execution details to react (#7063)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants