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(component): add expandable rows #579

Merged
merged 6 commits into from
Aug 27, 2021

Conversation

jorgemoya
Copy link
Contributor

@jorgemoya jorgemoya commented Aug 20, 2021

What?

Add expandable rows functionality to Worksheet.

Why?

Per the feature request.

I did, however, move the buttons to their own column to handle keyboard navigation properly. I created a ToggleEditor component which isn't really an "Editor", but keeps it together with all the other components that render items inside the cells. This enabled me to listen to keyboard events succesfully.

Screenshots/Screen Recordings

Kapture 2021-08-24 at 12 26 57

Testing/Proof

Unit tests and locally.

@jorgemoya jorgemoya requested review from a team as code owners August 20, 2021 16:48
@eugene-polev
Copy link

eugene-polev commented Aug 24, 2021

I'd like to propose several changes from the design perspective:

  1. Change + and - icons to "chevron right" and "chevron down" accordingly
  2. Make the first column narrower to hug the action icon for collapse/expand
  3. Remove the border between the first column with collapse/expand action and the column with the name
  4. Use white background for the header row (grey one creates not a very smooth transition between table header and page background - see in Figma)
  5. Use a larger indent for expanded rows instead of background color (the same issue as with the header + this pattern is not scalable for cases with more than one hierarchical level)

Figma file

image

@jorgemoya
Copy link
Contributor Author

jorgemoya commented Aug 24, 2021

Great feedback @eugene-polev. To maintain scope of this PR, I have updated the icons and the column size, but we should move the rest of the planned changes to a separate PR. This will allow us to be certain of all the changes needed and to schedule time for those changes.

@bc-jennlindeman
Copy link

Thanks @eugene-polev and @jorgemoya. I'm going to pass this PR since you're going to address his other feedback in another one.

@bc-jennlindeman
Copy link

Currently requesting access to product-design assignment.

Copy link
Contributor

@chanceaclark chanceaclark left a comment

Choose a reason for hiding this comment

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

Few comments but looks really good! Great job on this.

packages/big-design/src/components/Worksheet/utils.ts Outdated Show resolved Hide resolved
/>,
);

expect(screen.queryAllByRole('row').length).toBe(7);
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of testing for length here, could we check for visibility of fields in the table/worksheet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't querying and found a test that something was visible? Are you suggesting I test each row? Any guidance on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure this will work, but maybe something like this?

expect(screen.queryByRole('row', { name: /shoes name one/i })).not.toBeInTheDocument();

I'm mostly thinking of this part of RTL's documentation: https://testing-library.com/docs/guide-disappearance

packages/big-design/src/components/Worksheet/Row/styled.ts Outdated Show resolved Hide resolved
@@ -11,7 +11,7 @@ interface StyledCellProps<Item> {
}

export const StyledCell = styled.td<StyledCellProps<WorksheetItem>>`
background-color: ${({ theme }) => theme.colors.white};
background-color: ${({ theme }) => theme.colors.inherit};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I inherit now from the row.

/>,
);

expect(screen.queryAllByRole('row').length).toBe(7);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't querying and found a test that something was visible? Are you suggesting I test each row? Any guidance on this?

packages/big-design/src/components/Worksheet/utils.ts Outdated Show resolved Hide resolved
@jorgemoya jorgemoya merged commit aea894e into bigcommerce:master Aug 27, 2021
@jorgemoya jorgemoya deleted the expandable-rows branch August 27, 2021 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants