feat(checkBox): updated PF4 checkBox component according to PF-next #717

rvsia · 2018-10-08T09:48:37Z

What:

adds label for checkbox
adds validation for id, aria-label

checkBox must have an id
if label is not a string, checkBox must have an aria-label

according to https://pf-next.com/components/Check/examples/

Link to storybook::

https://717-pr-patternfly-react-patternfly.surge.sh/patternfly-4/components/checkbox/

Additional issues:

How should the component handle different additional attributes for different parts of the component?

e. g. Should we let users to add styles for wrapperDiv, label, input separately?
Right now, a consumer can add props only to input.

patternfly-build · 2018-10-08T10:01:07Z

PatternFly-React preview: https://717-pr-patternfly-react-patternfly.surge.sh

coveralls · 2018-10-08T10:01:26Z

Pull Request Test Coverage Report for Build 2558

1 of 2 (50.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.004%) to 80.79%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/patternfly-4/react-core/src/components/Checkbox/Checkbox.js	1	2	50.0%

Totals
Change from base Build 2547:	-0.004%
Covered Lines:	3349
Relevant Lines:	3900

💛 - Coveralls

tlabaj · 2018-10-08T14:10:14Z

packages/patternfly-4/react-core/src/components/Checkbox/Checkbox.js

+};
+
+const renderLabel = (label, checkId, isDisabled) => {
+  if (typeof label === 'function') {


It looks like we are only applying PF styles when the label is a string. Can we apply style to this regardless of type? I am concerned that the labels may be unstyled, leading to inconsistencies.

Agreed with this. If we really need to support replacing the label component used here (for now just an html label) we should follow something like labelComponent={CustomLabel} or compoents={{ Label: CustomLabel }} that would then get passed the className, htmlFor, or an other props.

If a basic function is needed the prop should be renderLabel and it should be passed the htmlFor and className prop to allow the user access to the styles.

I am concerned that the labels may be unstyled, leading to inconsistencies.

@tlabaj that is the point of these options. Reason why i wanted @rvsia to support different types of labels (nodes, function) is actually that you can have some other component (icons, pictures, w/e). If you want consistent/pf-styled label, then you should use the string option. Passing the styles to custom labels may actually break them. Whole reason of this approach is to give devs freedom. Yes they will have to be careful and style non-pf components on their own, but consistency of some app should a concern of the actual end user not the library.

@dmiller9911

Maybe I don't understand but I can't see any difference between this and your solution except the amount of props.

If user wants to use a component with props instead of the label he can do something like
<Checkbox label={<Customcomponent className="..." htmlFor="..." />}

I was more indicating that the function support should be removed. It does not add any value. It only complicates things. It should be node (covers string, rendered element, number, etc) only. That would support the follow

<Checkbox label="<CustomLabel />" /> <Checkbox label="Label" /> const renderLabel =() => 'Some Label'; <Checkbox label={renderLabel()}

The only reason we would want to pass a function to label would be to pass the className and any a11y props to it

const renderLabel = ({ className, htmlFor }) => ( <label className={`${className} ${someCustomClass}` } htmlFor={htmlFor}>Label Text</label> ); <Checkbox label={renderLabel}

Otherwise it is no differnet than the last example in the first block.

I agree with going with node here and removing the function support.

dmiller9911 · 2018-10-08T14:28:50Z

packages/patternfly-4/react-core/src/components/Checkbox/Checkbox.d.ts


 export interface CheckboxProps
  extends Omit<HTMLProps<HTMLInputElement>, 'type' | 'onChange' | 'disabled'> {
  isDisabled?: boolean;
  isValid?: boolean;
  onChange?(checked: boolean, event: FormEvent<HTMLInputElement>): void;
+  id?: string;
+  'aria-label'?: string;
+  label?: OneOf<typeof TextVariants, keyof typeof TextVariants>;


This is more easily written as string | ReactNode | Function. That being said I am curious why function is needed at all. Is there a usecase for it. I understand using render functions, but for a simple label is seems to add complications where they are not needed.

dmiller9911 · 2018-10-08T14:30:59Z

packages/patternfly-4/react-core/src/components/Checkbox/Checkbox.js

+    return null;
+  },
+  /** Aria-label of the checkbox. */
+  'aria-label': props => {


If using something like react-intl or react-i18next this will likely always error. I think we should just check for the existence of it since we are allowing nodes in addition to strings.

packages/patternfly-4/react-core/src/components/Checkbox/Checkbox.js

dmiller9911 · 2018-10-08T14:36:17Z

packages/patternfly-4/react-core/src/components/Checkbox/Checkbox.js

+          aria-invalid={!isValid}
+          disabled={isDisabled}
+        />
+        {label && renderLabel(label, props.id, isDisabled)}


If we get rid of the function (i think we should). Inlining the label down here would be easier to read and follow.

I think it (with all of the conditions) would be too complex to put into JSX.

I agree with getting rid of the function here to simplify this

dmiller9911 · 2018-10-08T14:43:14Z

packages/patternfly-4/react-core/src/components/Checkbox/Checkbox.js

+};
+
+const renderLabel = (label, checkId, isDisabled) => {
+  if (typeof label === 'function') {


Agreed with this. If we really need to support replacing the label component used here (for now just an html label) we should follow something like labelComponent={CustomLabel} or compoents={{ Label: CustomLabel }} that would then get passed the className, htmlFor, or an other props.

If a basic function is needed the prop should be renderLabel and it should be passed the htmlFor and className prop to allow the user access to the styles.

priley86 · 2018-10-08T18:53:18Z

*would you mind formatting the commit semantically ? ... you can use yarn commit to walk through the CLI.

jschuler · 2018-10-11T15:12:59Z

packages/patternfly-4/react-core/src/components/Checkbox/Checkbox.js

+  /** Label text of the checkbox. */
+  label: PropTypes.oneOfType([PropTypes.string, PropTypes.node, PropTypes.func]),
+  /** Id of the checkbox. */
+  id: props => {


I think you can just do id: PropTypes.string.isRequired

Agreed, this would simplify it and give the same error if not specified

jschuler

Agree with others that we should probably simplify label prop and remove function

jschuler · 2018-10-11T15:14:19Z

packages/patternfly-4/react-core/src/components/Checkbox/Checkbox.js

-  onChange: () => undefined
+  onChange: () => undefined,
+  label: undefined,
+  id: null,


If this is a string perhaps default to ''

packages/patternfly-4/react-core/src/components/Checkbox/Checkbox.js

packages/patternfly-4/react-core/src/components/Checkbox/Checkbox.test.js

packages/patternfly-4/react-core/src/components/Checkbox/examples/UncontrolledCheckbox.js

dmiller9911 · 2018-10-12T14:08:06Z

packages/patternfly-4/react-core/src/components/Checkbox/Checkbox.js

+          disabled={isDisabled}
+        />
+        {React.isValidElement(label)
+          ? label


this is still an issue in my mind, but maybe @amarie401 or @jschuler can weigh in on if it will cause them any problems. If someone provides

import { FormattedMessage } from 'react-intl'; const CustomCheckbox = (props) => ( <Checkbox label={<FormattedMessage defaultMessage="Label Value" id="checkbox.label" />} {...props} /> )

The label will go completely unstyled. There is also no way currently to use the label inside this as a component to make sure that I can wrap it any other way. to get around this I would have to do

import { injectIntl, defineMessages } from 'react-intl'; const messages = defineMessages({ label: { defaultMessage: 'Label Value', id: 'checkbox.label' } }) const CustomCheckbox = ({ intl, ...props }) => ( <Checkbox label={intl.formatMessage(messages.label)} {...props} /> ) const Wrapped = injectIntl(CustomCheckbox);

I propose that this return section is just changed to

<label className={css(styles.checkLabel, isDisabled && styles.modifiers.disabled)} htmlFor={props.id} > {label} </label>

That removes the string and element checks and just passes the node into the label.

I agree with Doug here. I think this leaves room for a label to be unstyled which could lead to an inconsistent user experience.

dmiller9911 · 2018-10-12T14:08:42Z

packages/patternfly-4/react-core/src/components/Checkbox/Checkbox.d.ts

 import { Omit } from '../../typeUtils';

 export interface CheckboxProps
  extends Omit<HTMLProps<HTMLInputElement>, 'type' | 'onChange' | 'disabled'> {
  isDisabled?: boolean;
  isValid?: boolean;
  onChange?(checked: boolean, event: FormEvent<HTMLInputElement>): void;
+  id: string;
+  'aria-label': string;
+  label?: string | ReactNode;


ReactNode includes string. This can just be label?: ReactNode

…demo

dmiller9911

Looks good! Thanks for the contribution and work through the implementation with us.

jschuler · 2018-10-13T13:33:54Z

Nice job @rvsia, thanks for your contribution! 🎉

rvsia · 2018-10-13T13:58:01Z

Thanks everyone for helping me! 👍

rvsia changed the title ~~feat(checkBox): updated checkBox component according to PF-next~~ feat(checkBox): updated PF4 checkBox component according to PF-next Oct 8, 2018

tlabaj added the PF4 label Oct 8, 2018

tlabaj reviewed Oct 8, 2018

View reviewed changes

amarie401 self-requested a review October 8, 2018 14:38

dmiller9911 suggested changes Oct 8, 2018

View reviewed changes

priley86 added the enhancement 🚀 label Oct 8, 2018

LHinson added this to the 1.0.0-alpha.4 milestone Oct 8, 2018

rvsia force-pushed the check-group branch 4 times, most recently from 131c55a to 6cf7ff4 Compare October 11, 2018 10:48

jschuler reviewed Oct 11, 2018

View reviewed changes

rvsia force-pushed the check-group branch 4 times, most recently from 6763cbd to 494e96a Compare October 12, 2018 10:10

jschuler previously approved these changes Oct 12, 2018

View reviewed changes

dmiller9911 suggested changes Oct 12, 2018

View reviewed changes

dmiller9911 mentioned this pull request Oct 12, 2018

feat(formComponent): add Form, FormGroup, ActionGroup PF4 components #667

Merged

feat(checkboxComponent): updated checkbox PF4 component according to …

ea2c445

…demo

rvsia dismissed jschuler’s stale review via ea2c445 October 12, 2018 16:10

rvsia force-pushed the check-group branch from 494e96a to ea2c445 Compare October 12, 2018 16:10

amarie401 approved these changes Oct 12, 2018

View reviewed changes

tlabaj approved these changes Oct 12, 2018

View reviewed changes

dmiller9911 approved these changes Oct 13, 2018

View reviewed changes

jschuler merged commit de120e3 into patternfly:master Oct 13, 2018

rvsia deleted the check-group branch October 13, 2018 13:58

rvsia mentioned this pull request Oct 15, 2018

feat(radioComponent): updated according to PF-next demo #756

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(checkBox): updated PF4 checkBox component according to PF-next #717

feat(checkBox): updated PF4 checkBox component according to PF-next #717

rvsia commented Oct 8, 2018 •

edited

Loading

patternfly-build commented Oct 8, 2018

coveralls commented Oct 8, 2018 •

edited

Loading

tlabaj Oct 8, 2018

dmiller9911 Oct 8, 2018

Hyperkid123 Oct 9, 2018

rvsia Oct 11, 2018

dmiller9911 Oct 11, 2018

amarie401 Oct 11, 2018

dmiller9911 Oct 8, 2018

dmiller9911 Oct 8, 2018

dmiller9911 Oct 8, 2018

rvsia Oct 11, 2018

amarie401 Oct 11, 2018

dmiller9911 Oct 8, 2018

priley86 commented Oct 8, 2018

jschuler Oct 11, 2018

amarie401 Oct 11, 2018

jschuler left a comment

jschuler Oct 11, 2018

dmiller9911 Oct 12, 2018

tlabaj Oct 12, 2018

jschuler Oct 12, 2018

dmiller9911 Oct 12, 2018

dmiller9911 left a comment

jschuler commented Oct 13, 2018

rvsia commented Oct 13, 2018

feat(checkBox): updated PF4 checkBox component according to PF-next #717

feat(checkBox): updated PF4 checkBox component according to PF-next #717

Conversation

rvsia commented Oct 8, 2018 • edited Loading

patternfly-build commented Oct 8, 2018

coveralls commented Oct 8, 2018 • edited Loading

Pull Request Test Coverage Report for Build 2558

💛 - Coveralls

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

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

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

priley86 commented Oct 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jschuler left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmiller9911 left a comment

Choose a reason for hiding this comment

jschuler commented Oct 13, 2018

rvsia commented Oct 13, 2018

rvsia commented Oct 8, 2018 •

edited

Loading

coveralls commented Oct 8, 2018 •

edited

Loading