search

pbylicki / rfhub2

Rewrite of rfhub with FastAPI
Apache License 2.0
30 stars 10 forks source link

Sorting Collection Table #450

Open MaciejWiczk opened 2 years ago

MaciejWiczk commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

@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.

  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.
MaciejWiczk commented 2 years ago

@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 commented 2 years ago

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