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

fix(structured-list): remove unused prop #5592

Merged
merged 4 commits into from
Mar 13, 2020
Merged

fix(structured-list): remove unused prop #5592

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d76eacf
fix(structured-list): remove unused prop
tw15egan Mar 10, 2020
48785ac
fix(structured-list): wrap prop in deprecate block
tw15egan Mar 10, 2020
8817e43
fix(structured-list): remove border from other
tw15egan Mar 11, 2020
0974603
Merge branch 'master' into structured-list-prop-removal
joshblack Mar 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/react/src/components/StructuredList/StructuredList-story.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ storiesOf('StructuredList', module)
));
};
return (
<StructuredListWrapper selection border>
<StructuredListWrapper selection>
<StructuredListHead>
<StructuredListRow head>
<StructuredListCell head>ColumnA</StructuredListCell>
Expand Down Expand Up @@ -125,7 +125,7 @@ storiesOf('StructuredList', module)
() => (
<div style={{ width: '800px' }}>
<StructuredListSkeleton />
<StructuredListSkeleton border />
<StructuredListSkeleton />
</div>
),
{
Expand Down
15 changes: 0 additions & 15 deletions packages/react/src/components/StructuredList/StructuredList-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,27 +33,12 @@ describe('StructuredListWrapper', () => {
expect(wrapper.hasClass('extra-class')).toEqual(true);
});

it('By default, border prop is false', () => {
wrapper.setProps({ border: false });
expect(wrapper.hasClass(`${prefix}--structured-list--border`)).toEqual(
false
);
});

it('By default, selection prop is false', () => {
wrapper.setProps({ border: false });
expect(wrapper.hasClass(`${prefix}--structured-list--selection`)).toEqual(
false
);
});

it('Should add the modifier class for border when border prop is true', () => {
wrapper.setProps({ border: true });
expect(wrapper.hasClass(`${prefix}--structured-list--border`)).toEqual(
true
);
});

it('Should add the modifier class for selection when selection prop is true', () => {
wrapper.setProps({ selection: true });
expect(wrapper.hasClass(`${prefix}--structured-list--selection`)).toEqual(
Expand Down
16 changes: 1 addition & 15 deletions packages/react/src/components/StructuredList/StructuredList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,6 @@ export class StructuredListWrapper extends Component {
*/
className: PropTypes.string,

/**
* Specify whether a border should be added to your StructuredListWrapper
*/
border: PropTypes.bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we might need to wrap this in a deprecate block as lame as that is otherwise if someone is passing in border it might get spread onto <section> with other accidentally 😞 This could cause React to complain about the extra attribute showing up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

😞

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do we need to keep the rest of the border code as well? Or is keeping the prop definition wrapped in a deprecate enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

@tw15egan I doubt we need to keep the class name code. As long as we have:

    const { children, selection, className, ariaLabel, border: _border, ...other } = this.props;

I think it'd be fine so it doesn't get caught up in other. The rest of the code can totally be dropped I think (alongside the deprecate bit for the prop type)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alright I've updated it, let me know if the changes look good 👍


/**
* Specify whether your StructuredListWrapper should have selections
*/
Expand All @@ -42,23 +37,14 @@ export class StructuredListWrapper extends Component {
};

static defaultProps = {
border: false,
selection: false,
ariaLabel: 'Structured list section',
};

render() {
const {
children,
selection,
className,
border,
ariaLabel,
...other
} = this.props;
const { children, selection, className, ariaLabel, ...other } = this.props;

const classes = classNames(`${prefix}--structured-list`, className, {
[`${prefix}--structured-list--border`]: border,
[`${prefix}--structured-list--selection`]: selection,
});

Expand Down