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

Build TreeView for SortBy #687

Merged
merged 56 commits into from
Aug 11, 2021
Merged

Build TreeView for SortBy #687

merged 56 commits into from
Aug 11, 2021

Conversation

rogermparent
Copy link
Contributor

@rogermparent rogermparent commented Jul 27, 2021

multi-sort-tree-demo.mp4

This PR adds a TreeView for viewing, adding, and removing sorts.

It also adds the ability for multiple sorts to be applied, as that's pretty inherent to the whole purpose of the tree such that most of its commands couldn't even be implemented without multi-sort.
This feature makes the assumption that users will only want one sort on any particular column, because the alternatives, sorting one direction and then the other or sorting in the same direction twice, don't really make any sense.

There's also a large sort tree integration test, which is really more of a general sorting integration test other than the fact it uses the table-specific sort commands alongside the more general ones.

The PR is pretty big, but most of the lines are tests, particularly ~300 from the new integration test.

Tasks for the future:

  • Test the scenario of multiple projects in the integration test, as the behavior is very different and the tree commands aren't even exposed with just one dvc repo.
  • Add inline commands to move sorts (or drag-and-drop if possible), and reverse the direction of an individual sort (could've been done here but the functionality can be done with add/remove and this PR's big enough already)

@rogermparent rogermparent self-assigned this Jul 27, 2021
@rogermparent rogermparent added A: experiments Area: experiments table webview and everything related product PR that affects product labels Jul 27, 2021
- Split getChildren branches into methods
- Await experiments for root
- Turn getSort into getSorts (multi sorts not actually implemented)
@rogermparent rogermparent requested a review from mattseddon July 29, 2021 04:19
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.

Looks good. The only thing I think you need to change is the treeItem Id and add a way to map back to an experiments repository. Doesn't have to be in this PR

extension/src/experiments/model/sortBy/tree.test.ts Outdated Show resolved Hide resolved
extension/src/experiments/model/sortBy/tree.ts Outdated Show resolved Hide resolved
extension/src/experiments/model/sortBy/tree.ts Outdated Show resolved Hide resolved
extension/src/experiments/model/sortBy/tree.ts Outdated Show resolved Hide resolved
@rogermparent rogermparent marked this pull request as ready for review August 6, 2021 21:58
@rogermparent rogermparent requested a review from mattseddon August 6, 2021 22:19
extension/package.json Outdated Show resolved Hide resolved
extension/package.json Outdated Show resolved Hide resolved
extension/src/experiments/index.ts Outdated Show resolved Hide resolved
extension/src/experiments/model/sortBy/tree.ts Outdated Show resolved Hide resolved
extension/src/experiments/model/sortBy/tree.ts Outdated Show resolved Hide resolved
extension/src/experiments/model/sortBy/tree.ts Outdated Show resolved Hide resolved
extension/src/experiments/model/sortBy/tree.ts Outdated Show resolved Hide resolved
@codeclimate
Copy link

codeclimate bot commented Aug 11, 2021

Code Climate has analyzed commit 60cbee1 and detected 0 issues on this pull request.

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

This pull request will bring the total coverage in the repository to 94.7% (0.4% change).

View more on Code Climate.

@mattseddon mattseddon merged commit 62a723c into master Aug 11, 2021
@mattseddon mattseddon deleted the sort-by-tree branch August 11, 2021 22:43
@rogermparent rogermparent mentioned this pull request Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: experiments Area: experiments table webview and everything related product PR that affects product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants