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

Allow sorting apps by store, name or app type #1753

Merged
merged 2 commits into from
Jul 28, 2020

Conversation

bolatovumar
Copy link
Contributor

See in action:

Jul-19-2020 14-14-25

Also has helpful tooltips on hover:

Screen Shot 2020-07-19 at 2 08 07 PM
Screen Shot 2020-07-19 at 2 08 29 PM

I modelled the behavior after the macOS sort functionality in Finder:

Screen Shot 2020-07-19 at 2 25 26 PM

close #1568

Copy link
Member

@Kukks Kukks left a comment

Choose a reason for hiding this comment

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

I think @rockstardev 's PR of concolidating the list views will conflict with this. Let's wait till it is merged and move this logic on to his foundation so that we can apply sorting to all columns for all lists

BTCPayServer/Controllers/AppsController.cs Outdated Show resolved Hide resolved
switch (sortOrder)
{
case "desc":
ViewData[$"{sortOrderColumn}SortOrder"] = "asc";
Copy link
Member

Choose a reason for hiding this comment

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

Maybe an enum fits better here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kukks you mean enum for "asc" and "desc" or for $"{sortOrderColumn}SortOrder"?

@rockstardev rockstardev self-requested a review July 20, 2020 08:00
Copy link
Member

@rockstardev rockstardev left a comment

Choose a reason for hiding this comment

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

I've glanced over PR and as @Kukks says it's a question on how we want to standardize this. @bolatovumar has added this only to Apps grid, so it doesn't conflict with my changes... but it's a question on which grids we want ordering... kinda question for @NicolasDorier or our designer wizards (cc @pavlenex).

In any case I would generalize this so that that we don't have to specify details on every grid. Also now that we have ability to remember user preferences in cookie, this would particularly be useful for sorting.

@dennisreimann
Copy link
Member

I think it makes sense to use this pattern everywhere, if @rockstardev can properly standardize it. (and I have no doubt that he can, because why wouldn't he? He's our rockstar.)
Being consistent would not just make it easier to understand from a users perspective, but it'd also make the code easier to grasp when all those list action use the same patterns.

@bolatovumar
Copy link
Contributor Author

bolatovumar commented Jul 21, 2020

As far as I see it there are two places where you would want to have sorting:

  1. Apps page
  2. Invoices page

Other pages (like "Stores", for example) probably won't have enough items to warrant sorting.

Also, I'm not sure how one would standardize this (I'm not rockstar after all) but I would be glad to implement something similar for the "Invoices" page if this gets merged.

@rockstardev
Copy link
Member

I'm so touched by your belief in me @dennisreimann & @bolatovumar. I promise not to let you down.

@rockstardev rockstardev self-assigned this Jul 24, 2020
@bolatovumar
Copy link
Contributor Author

@rockstardev i'm wondering if it's even worth it to spend time on making a generic solution for this as there maybe 2-3 pages which would use this functionality. I'm fine with it either way, just thinking about the trade-offs.

@rockstardev
Copy link
Member

@bolatovumar I'm waiting on @NicolasDorier to review #1754 and merge. Once he does that I'll just rebase this on top of that and take a look. To me it definitely makes sense to at least extract sorting logic out of controllers... generalization or no generalization.

And same for the <a> elements - typing all that stuff for every column is kinda cumbersome - would be much better to abstract it one level above... because everything that's different is Name and NameSortOrder key between elements.

@rockstardev
Copy link
Member

And yeah - when I was saying "generalization" - I was more meaning in the direction of having a standard UI pattern (what @dennisreimann is saying).

@bolatovumar
Copy link
Contributor Author

@rockstardev cool, makes sense.

@NicolasDorier
Copy link
Member

NicolasDorier commented Jul 27, 2020

I am not fan of generalization either.

As then they may decrease the time we spend changing something across many page, but require more effort of communication to be on the same page, increase the knowledge needed to fix something and increase the chance of collateral damage on something else.

For generalization of UI pattern, I agree on simple things. Maybe some specific stuff can make better use of ASP.NET Core components for better reuse. (For example, the pager could be a component with a specific viewmodel that we reuse everywhere)

Sorting may be done in a similar way. (though I don't know how)

Also maybe for some table, a sorting at the JS level may make more sense than at the controller level, so it is far from easy to generalize.

@NicolasDorier
Copy link
Member

@bolatovumar I am not fan of using ViewData for that, I think it should go on a ViewModel that we may be able to reuse elsewhere for controller side sorting.

Though I will merge, can try to fix that after.

@bolatovumar
Copy link
Contributor Author

Also maybe for some table, a sorting at the JS level may make more sense than at the controller level, so it is far from easy to generalize.

I was initially planning on doing this in JS but as @Kukks said in the original issue for this PR (#1568 (comment)) we have to do it on the backend since we have pagination here and we can't do sorting across multiple pages with JS (well, without creating an API endpoint to return data and whatnot, at least).

@NicolasDorier NicolasDorier merged commit b7d66ef into btcpayserver:master Jul 28, 2020
@bolatovumar bolatovumar deleted the feat/1568 branch July 29, 2020 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Apps - Sort by name (Store-Name-Apptype)
5 participants