FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(Dropdown split button): Add 3rd state to split button by karelhala · Pull Request #2842 · patternfly/patternfly-react · GitHub

/ patternfly-react Public

fix(Dropdown split button): Add 3rd state to split button #2842

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

Merged
tlabaj merged 2 commits into from
Sep 23, 2019
Conversation 12 Commits 2 Checks 0 Files changed
Merged

fix(Dropdown split button): Add 3rd state to split button #2842

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cabdf0a
fix(Dropdown split button): Add 3rd state to split button
karelhala Sep 5, 2019
14111cc
Use target as source of truth for checked
karelhala Sep 6, 2019
File filter

Filter by extension

Filter by extension .md  (1) .snap  (1) .tsx  (2) All 3 file types selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
73 changes: 73 additions & 0 deletions packages/patternfly-4/react-core/src/components/Dropdown/Dropdown.md
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -661,6 +661,79 @@ class SplitButtonDropdown extends React.Component {
}
```

## Split button (3rd state)

```js
import React from 'react';
import { Dropdown, DropdownToggle, DropdownToggleCheckbox, DropdownItem, DropdownSeparator, DropdownPosition, DropdownDirection, KebabToggle } from '@patternfly/react-core';
import { ThIcon } from '@patternfly/react-icons';
class SplitButtonDropdown extends React.Component {
constructor(props) {
super(props);
this.state = {
isOpen: false,
isChecked: null
};
this.onToggle = isOpen => {
this.setState({
isOpen
});
};
this.onSelect = event => {
this.setState({
isOpen: !this.state.isOpen
});
};
this.onChange = (isChecked) => {
this.setState({
isChecked
})
}
}
render() {
const { isOpen, isChecked } = this.state;
const dropdownItems = [
<DropdownItem key="link">Link</DropdownItem>,
<DropdownItem key="action" component="button">
Action
</DropdownItem>,
<DropdownItem key="disabled link" isDisabled>
Disabled Link
</DropdownItem>,
<DropdownItem key="disabled action" isDisabled component="button">
Disabled Action
</DropdownItem>,
<DropdownSeparator key="separator" />,
<DropdownItem key="separated link">Separated Link</DropdownItem>,
<DropdownItem key="separated action" component="button">
Separated Action
</DropdownItem>
];
return (
<Dropdown
onSelect={this.onSelect}
toggle={(
<DropdownToggle
splitButtonItems={[
<DropdownToggleCheckbox
id="example-checkbox-3rd-state"
key="split-checkbox"
aria-label="Select all"
onChange={(checked) => this.onChange(checked)}
isChecked={isChecked}
/>
]}
onToggle={this.onToggle}
/>
)}
isOpen={isOpen}
dropdownItems={dropdownItems}
/>
);
}
}
```

## Split button (disabled)

```js
Expand Down
14 changes: 10 additions & 4 deletions packages/patternfly-4/react-core/src/components/Dropdown/DropdownToggleCheckbox.test.tsx
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as React from 'react';
import { shallow } from 'enzyme';
import { shallow, mount } from 'enzyme';
import { DropdownToggleCheckbox } from './DropdownToggleCheckbox';

const props = {
Expand Down Expand Up @@ -27,6 +27,11 @@ test('isDisabled', () => {
expect(view).toMatchSnapshot();
});

test('3rd state', () => {
const view = shallow(<DropdownToggleCheckbox id="check" isChecked={null} aria-label="check" />);
expect(view).toMatchSnapshot();
});

test('passing class', () => {
const view = shallow(
<DropdownToggleCheckbox label="label" className="class-123" id="check" isChecked aria-label="check" />
Expand All @@ -44,9 +49,10 @@ test('passing HTML attribute', () => {
test('checkbox passes value and event to onChange handler', () => {

karelhala Sep 5, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I had to remove this test because jest is not playing nicely with mount and currentTarget enzymejs/enzyme#218.

I had to use mount because checkbox is now in Checkbox component.

redallen Sep 5, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I've run into this same issue (I think also with Checkbox). My solution was to change handleChange in Checkbox.tsx from:

private handleChange = (event: React.FormEvent<HTMLInputElement>): void => {
  this.props.onChange(event.currentTarget.checked, event);
}

to:

private handleChange = (event: React.FormEvent<HTMLInputElement>): void => {
  this.props.onChange(event.target.checked, event);
}

If Checkbox doesn't break, feel free to try that.

redallen Sep 5, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think we should open a high-priority issue to add this test back if we aren't willing to address it in the scope of this PR.

karelhala Sep 6, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yeah, that came up in head as well, I didn't wanted to change the original Checkbox though because it's not part of this PR. But I'll probably change it to use target instead of currentTarget.

I could also change handleChange of this component to use target instead of currentTarget which is probably a bit safer.
const newValue = true;
const event = {
currentTarget: { checked: newValue }
target: { checked: newValue }
};
const view = shallow(<DropdownToggleCheckbox id="check" {...props} aria-label="check" />);
const view = mount(<DropdownToggleCheckbox id="check" {...props} aria-label="check" />);
view.find('input').simulate('change', event);
expect(props.onChange).toBeCalledWith(newValue, event);
expect(props.onChange.mock.calls[0][0]).toBe(newValue);
expect(props.onChange.mock.calls[0][1]).toMatchObject(event);
});
27 changes: 16 additions & 11 deletions packages/patternfly-4/react-core/src/components/Dropdown/DropdownToggleCheckbox.tsx
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import * as React from 'react';
import styles from '@patternfly/react-styles/css/components/Dropdown/dropdown';
import { css } from '@patternfly/react-styles';
import { Checkbox } from '../Checkbox';
import { Omit } from '../../helpers/typeUtils';

export interface DropdownToggleCheckboxProps extends Omit<React.HTMLProps<HTMLInputElement>, 'type' | 'onChange' | 'disabled' | 'checked'> {
export interface DropdownToggleCheckboxProps
extends Omit<React.HTMLProps<HTMLInputElement>, 'type' | 'onChange' | 'disabled' | 'checked'> {
/** Additional classes added to the DropdownToggleCheckbox */
className?: string;
/** Flag to show if the checkbox selection is valid or invalid */
Expand All @@ -30,17 +32,20 @@ export class DropdownToggleCheckbox extends React.Component<DropdownToggleCheckb
className: '',
isValid: true,
isDisabled: false,
isChecked: null as boolean | null,
checked: null as boolean | null,
onChange: () => undefined as any
};

handleChange = (event: React.ChangeEvent<HTMLInputElement>) => {
this.props.onChange(event.currentTarget.checked, event);
handleChange = (checked: boolean, event: React.FormEvent<HTMLInputElement>) => {
this.props.onChange((event.target as HTMLInputElement).checked, event);
}

calculateChecked = () => {
const { isChecked, checked } = this.props;
return isChecked !== undefined ? isChecked : checked;
}

render() {
const { className, onChange, isValid, isDisabled, isChecked, checked, children, ...props } = this.props;
const { className, onChange, isValid, isDisabled, isChecked, ref, checked, children, ...props } = this.props;
const text = children && <span
className={css(styles.dropdownToggleText, className)}
aria-hidden="true"
Expand All @@ -50,13 +55,13 @@ export class DropdownToggleCheckbox extends React.Component<DropdownToggleCheckb
</span>;
return (
<label className={css(styles.dropdownToggleCheck, className)} htmlFor={props.id}>
<input
<Checkbox
{...props}
type="checkbox"
onChange={this.handleChange}
{...(this.calculateChecked() !== undefined) && { onChange: this.handleChange }}
ref={ref as any}

karelhala Sep 5, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Anybody knows something better to use here?

redallen Sep 5, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why do you have to pass the ref here? It will be passed automatically with {...props} with the correct type.

karelhala Sep 6, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It's because TS does not recognized the ref correctly in here. I tried casting it to expected and it failed. This was the only solution that didn't thre any error.
aria-invalid={!isValid}
disabled={isDisabled}
defaultChecked={isChecked || checked}
isDisabled={isDisabled}
isChecked={this.calculateChecked()}
/>
{text}
</label>
Expand Down
72 changes: 48 additions & 24 deletions ...y-4/react-core/src/components/Dropdown/__snapshots__/DropdownToggleCheckbox.test.tsx.snap
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff line change
@@ -1,18 +1,37 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`3rd state 1`] = `
<label
className="pf-c-dropdown__toggle-check"
htmlFor="check"
>
<Checkbox
aria-invalid={false}
aria-label="check"
className=""
id="check"
isChecked={null}
isDisabled={false}
isValid={true}
onChange={[Function]}
/>
</label>
`;

exports[`controlled 1`] = `
<label
className="pf-c-dropdown__toggle-check"
htmlFor="check"
>
<input
<Checkbox
aria-invalid={false}
aria-label="check"
defaultChecked={true}
disabled={false}
className=""
id="check"
isChecked={true}
isDisabled={false}
isValid={true}
onChange={[Function]}
type="checkbox"
/>
</label>
`;
Expand All @@ -22,14 +41,15 @@ exports[`isDisabled 1`] = `
className="pf-c-dropdown__toggle-check"
htmlFor="check"
>
<input
<Checkbox
aria-invalid={false}
aria-label="check"
defaultChecked={null}
disabled={true}
className=""
id="check"
isChecked={false}
isDisabled={true}
isValid={true}
onChange={[Function]}
type="checkbox"
/>
</label>
`;
Expand All @@ -39,16 +59,17 @@ exports[`passing HTML attribute 1`] = `
className="pf-c-dropdown__toggle-check"
htmlFor="check"
>
<input
<Checkbox
aria-invalid={false}
aria-label="check"
aria-labelledby="labelId"
defaultChecked={true}
disabled={false}
className=""
id="check"
isChecked={true}
isDisabled={false}
isValid={true}
label="label"
onChange={[Function]}
type="checkbox"
/>
</label>
`;
Expand All @@ -58,15 +79,16 @@ exports[`passing class 1`] = `
className="pf-c-dropdown__toggle-check class-123"
htmlFor="check"
>
<input
<Checkbox
aria-invalid={false}
aria-label="check"
defaultChecked={true}
disabled={false}
className=""
id="check"
isChecked={true}
isDisabled={false}
isValid={true}
label="label"
onChange={[Function]}
type="checkbox"
/>
</label>
`;
Expand All @@ -76,14 +98,15 @@ exports[`uncontrolled 1`] = `
className="pf-c-dropdown__toggle-check"
htmlFor="check"
>
<input
<Checkbox
aria-invalid={false}
aria-label="check"
defaultChecked={null}
disabled={false}
className=""
id="check"
isChecked={false}
isDisabled={false}
isValid={true}
onChange={[Function]}
type="checkbox"
/>
</label>
`;
Expand All @@ -93,14 +116,15 @@ exports[`with text 1`] = `
className="pf-c-dropdown__toggle-check"
htmlFor="check"
>
<input
<Checkbox
aria-invalid={false}
aria-label="check"
defaultChecked={null}
disabled={true}
className=""
id="check"
isChecked={false}
isDisabled={true}
isValid={true}
onChange={[Function]}
type="checkbox"
/>
<span
aria-hidden="true"
Expand Down
Back | FazBrowse Home | New Git URL