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

Sorting Collection Table #450

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Sorting Collection Table #450

wants to merge 12 commits into from

Conversation

MaciejWiczk
Copy link
Collaborator

resolves #146
I just extended frontend bit from @MarcinMaciaszek, but this is outside our areas of expertise ;)
One side effect I noticed is that left side panel get's sorted as well, when using sort on Collection Table:
Screencast from 03 12 2021 12_52_56

@MaciejWiczk MaciejWiczk added enhancement New feature or request python Pull requests that update Python code javascript Pull requests that update Javascript code labels Dec 3, 2021
@MaciejWiczk MaciejWiczk self-assigned this Dec 3, 2021
@MaciejWiczk
Copy link
Collaborator Author

@pbylicki one unit test is failing and I can't get why. There is some error on mapping for Collection on update. Functional tests are passing, because CLI is not using update functionality.
Ideally, this whole PR would need your review.

@pbylicki
Copy link
Owner

pbylicki commented Dec 5, 2021

@pbylicki one unit test is failing and I can't get why. There is some error on mapping for Collection on update. Functional tests are passing, because CLI is not using update functionality. Ideally, this whole PR would need your review.

@MaciejWiczk I see two problems here that ideally could be solved in separate PRs.

  1. Backend changes - I think there are more changes here than required by PR scope. If I understand correctly, you want to
  • add keywords count to _items_with_stats query in Collections (I don't think it should be added to _items without stats, UI is not using it and I think at some point long time ago we already considered this endpoint deprecated)
  • then handle new column in from_stats_row and add it to CollectionWithStats model
  • add calculated columns to ordering_criteria by defining custom_column_mapping

Most of these changes are already present, but there are some additional parts that are not clear for me.
2. Frontend changes, no issues with the work on sorting the table, but IMO the side-effect is pretty bad and it should be solved by splitting the underlying data that both views are using so that drawer uses constant, default ordering and only table allows to change ordering.

@MaciejWiczk
Copy link
Collaborator Author

MaciejWiczk commented Dec 6, 2021

@pbylicki

@MaciejWiczk I see two problems here that ideally could be solved in separate PRs.

  1. Backend changes - I think there are more changes here than required by PR scope. If I understand correctly, you want to
  • add keywords count to _items_with_stats query in Collections (I don't think it should be added to _items without stats, UI is not using it and I think at some point long time ago we already considered this endpoint deprecated)

Yes, currently we are using FE to get length of keywords array:
<TableCell align="right">{collection.keywords.length}</TableCell>.
Maybe it would be good to relieve fronted of that?
Also CLI is using that endpoint, but that would be an easy change to make :)

  • then handle new column in from_stats_row and add it to CollectionWithStats model

Yes, it's added to Collection, with FE somewhere change in my mind ;)

  • add calculated columns to ordering_criteria by defining custom_column_mapping

Most of these changes are already present, but there are some additional parts that are not clear for me.

TBH I tried to make this work, I'm not fluent enough here, to grasp the proper way.

  1. Frontend changes, no issues with the work on sorting the table, but IMO the side-effect is pretty bad and it should be solved >by splitting the underlying data that both views are using so that drawer uses constant, default ordering and only table allows >to change ordering.

Two requests to be made separate (async), or to use store? What would You suggest?

@pbylicki
Copy link
Owner

pbylicki commented Dec 7, 2021

Added PR to isolate required backend changes: #451
Will take a look into frontend issue in the next couple of days

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request javascript Pull requests that update Javascript code python Pull requests that update Python code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add sorting for Collections table
3 participants