Skip to content

Conversation

@matejminar
Copy link
Member

Kapture 2020-12-14 at 17 12 55

We are using this pattern already in multiple places and each has its own implementation.
In the follow-up PR, I will replace all of them with this reusable one.

@matejminar matejminar requested a review from a team December 14, 2020 16:15
const {collapseButton} = this.props;

if (typeof collapseButton === 'function') {
return collapseButton({handleCollapse: this.onCollapseToggle});
Copy link
Member

Choose a reason for hiding this comment

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

We should swap the prefixes here, the callback should be onCollapse and the method should be this.handleCollapseToggle.

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch, will do

Comment on lines 17 to 19
expect(wrapper.find('Button[data-test-id="expand"]').text()).toBe(
'Show 2 collapsed items'
);
Copy link
Member

Choose a reason for hiding this comment

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

imo we shouldn't use data-test-id for Buttons, we should instead use aria-label as that'll build good habits to ensure that our buttons are accessible.

Copy link
Member Author

Choose a reason for hiding this comment

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

sure thing, let me switch to that


return (
<React.Fragment>
{itemsToRender.map(item => item)}
Copy link
Member

Choose a reason for hiding this comment

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

Do you need this map? There are no changes/extraction being done.

@matejminar matejminar merged commit 71d0a82 into master Dec 15, 2020
@matejminar matejminar deleted the feat/collapsible-component branch December 15, 2020 17:40
@github-actions github-actions bot locked and limited conversation to collaborators Dec 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants