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

Collapse duplicate rows when table is flattened #4735

Merged
merged 26 commits into from
Oct 3, 2023
Merged

Conversation

julieg18
Copy link
Contributor

@julieg18 julieg18 commented Sep 28, 2023

Demo

Screen.Recording.2023-09-28.at.12.24.37.PM.mov

Part of #4746

@julieg18 julieg18 added the bug Something isn't working label Sep 28, 2023
@julieg18 julieg18 self-assigned this Sep 28, 2023
@julieg18 julieg18 mentioned this pull request Sep 28, 2023
1 task
Copy link
Contributor Author

@julieg18 julieg18 Sep 29, 2023

Choose a reason for hiding this comment

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

Updated our sorting fixture to have multiple branches:

  • main with 5 commits and seven experiments under one
  • other-branch with one commit and no experiments
  • another-branch with one commit and no experiments

@@ -183,46 +183,69 @@ describe('App', () => {
await expectHeaders(['A', 'C', 'D', 'B'])
})

it('should add a "branch/tags" column if the table is sorted', () => {
renderTable(sortedTableStateFixture)
describe('Sorted (Flattened) Table', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Grouped the "parents" column tests inside of a describe block.

>
<div className={styles.cellContents}>
<Icon icon={GitMerge} width={13} height={13} />
<ul
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could also use span but chose ul/li for accessibility.

@julieg18 julieg18 marked this pull request as ready for review September 29, 2023 16:42
Base automatically changed from flatten-table-on-sort to main September 29, 2023 22:34
@mattseddon
Copy link
Member

@julieg18 please rebase when you get a chance. I'll review afterwards.

@julieg18 julieg18 marked this pull request as draft September 30, 2023 00:16
@julieg18 julieg18 marked this pull request as ready for review September 30, 2023 00:30
@julieg18
Copy link
Contributor Author

@julieg18 please rebase when you get a chance. I'll review afterwards.

Done :)

Copy link
Member

@mattseddon mattseddon left a comment

Choose a reason for hiding this comment

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

I took a quick look and I think the data structure could be improved a little bit. I'll do a real review at the start of the working week 🙏🏻.

extension/src/experiments/model/index.ts Outdated Show resolved Hide resolved
extension/src/experiments/model/index.ts Outdated Show resolved Hide resolved
}
} = cell as unknown as CellContext<Experiment, CellValue>

if (!branch) {
if (flatBranches.length === 0) {
Copy link

Choose a reason for hiding this comment

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

Similar blocks of code found in 2 locations. Consider refactoring.

@mattseddon mattseddon enabled auto-merge (squash) October 2, 2023 21:47
@mattseddon mattseddon disabled auto-merge October 2, 2023 22:04
@mattseddon
Copy link
Member

Deeply nested sorting story needs to be updated :)

@julieg18 julieg18 enabled auto-merge (squash) October 3, 2023 13:19
@codeclimate
Copy link

codeclimate bot commented Oct 3, 2023

Code Climate has analyzed commit f819ec9 and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1
Duplication 2

The test coverage on the diff in this pull request is 100.0% (85% is the threshold).

This pull request will bring the total coverage in the repository to 95.1%.

View more on Code Climate.

@julieg18 julieg18 merged commit 625c471 into main Oct 3, 2023
@julieg18 julieg18 deleted the remove-duplicate-rows branch October 3, 2023 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants