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

Improve invoice filtering UI #4914

Merged
merged 7 commits into from May 19, 2023

Conversation

dennisreimann
Copy link
Member

Closes #3664.

Copy link
Contributor

@pavlenex pavlenex left a comment

Choose a reason for hiding this comment

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

tACK This is a good cleanup, concept ACK, will leave it up to @dstrukt to comment on UI, but imo this is great solution, good job!

Copy link
Member

@dstrukt dstrukt left a comment

Choose a reason for hiding this comment

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

Overall, love it, very clean change!

However, few suggestions:

  • Push the Export button (it's disappeared?) next to the Create button
  • Let the elements go full width (sans Export / Actions)
  • Rename Date to Time? Thoughts?
  • min-width the Status and Time buttons to 120px?
  • Reduced the left padding (looked better) to 16px vs. 24px?

i.e:
Screen Shot 2023-05-04 at 5 12 43 PM

  • Thoughts on pushing the mass actions to the next line? i.e:

Screen Shot 2023-05-04 at 8 55 51 PM

Do you think we could remove the Search button, and instead add Search... instead?

Otherwise, this looks great - awesome work! We could easily translate this to other views too.

@dennisreimann
Copy link
Member Author

Did the updates:

grafik

@dstrukt The export/actions buttons are only present when there are results, so I think the first point of that list doesn't apply?

@dstrukt
Copy link
Member

dstrukt commented May 5, 2023

Ahh yes, those changes are intended for if >1 result, just knew I could show what the search/time/status would look like sans Export/Actions via a screenshot/ browser editor to better communicate.

I've been wanting to improve the Actions button/CTA for a little while, so thought this might be a good time, but if you think it falls outside of the scope of this PR, feel free to ignore for now...

Screen Shot 2023-05-05 at 11 04 09 AM

Or if we wanted to do a dropdown menu from a carot for the mass actions...

Screen Shot 2023-05-05 at 6 35 49 PM

Also, for my understanding, are we not concerned about filtering by Plugin (formally App) anymore?

Otherwise, looks fantastic, love this update!

@dennisreimann
Copy link
Member Author

dennisreimann commented May 11, 2023

@dstrukt I added the app filter and also indicators on the top-level (dropdown button) to show which filter is active:

grafik

@dennisreimann
Copy link
Member Author

@dstrukt Regarding "Export button next to Create Invoice": We are a bit constraint here, because there are actually two HTML forms on this page: One for search/filtering and another one for export/archive. The latter has to include the checkboxes in the list and we cannot move the Export button out of the form.

Something like this would work though, if we move the Export button next to Archive again. This would also make it more logical imho, because we made it so that only the checked invoices get exported — so having a direct connection to the list would make sense for the user as well.

grafik

@dstrukt dstrukt self-requested a review May 13, 2023 23:29
@dstrukt
Copy link
Member

dstrukt commented May 14, 2023

Updated two snippets of text (Plugin instead of App) as we're moving to the plugins label, and Search... instead of Search ....

Overall it looks fantastic, this is all much easier to parse!

  • I still think we should try to address mass actions in a sep. PR.
  • I see where you're coming from with concerns for Export, so perhaps maybe we can add the export as part of the mass action, and arguably as you echo, it makes more sense, export 2 items or "all selected". I suppose if you have 5K invoices, and could only select 500, that might be a problem though...

The current implementation works perfectly well, but thought/suggestion for even more clarity wrt "selected" filters:
For the Status and Plugin filters, we could surface the active count to the parent text, e.g.

  • 2 Plugin(s) or 1 Plugin (if selected)
  • 3 Status or 1 Status (if selected)
    But for Time it could just change to the selected range, e.g simply:
  • 24 Hours, 3 Days, 7 Days or Custom
    Just a thought.

Otherwise, think we're ready to go!

@dstrukt dstrukt mentioned this pull request May 14, 2023
@dennisreimann
Copy link
Member Author

I suppose if you have 5K invoices, and could only select 500, that might be a problem though...

If there are none selected, everything will be exported.

suggestion for even more clarity wrt "selected" filters

Implemented it …

grafik

@dstrukt
Copy link
Member

dstrukt commented May 15, 2023

If there are none selected, everything will be exported.

Ahh, good to know, and something to consider with the mass action mock proposal, in its current form, in order to see the options at least 1 result needs to be selected.

Implemented it …

Looks great in the screenshot! will review this evening .. what do you think of it?

@dennisreimann
Copy link
Member Author

what do you think of it?

I think it gives even more overview, so good suggestion and full ACK 👌

Copy link
Member

@dstrukt dstrukt left a comment

Choose a reason for hiding this comment

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

LGTM!

Only change we could consider the new filtering update @dennisreimann is All Status All Plugins All Time as the default states, but I realize this adds more horizontal space .. so could be something we could address when we tackle Export / Actions stuff, but very minor.

@dennisreimann
Copy link
Member Author

All Status All Plugins All Time as the default states

Applied it :)

@dstrukt
Copy link
Member

dstrukt commented May 17, 2023

LGTM!

@NicolasDorier NicolasDorier merged commit 6c6544b into btcpayserver:master May 19, 2023
4 checks passed
@dennisreimann dennisreimann deleted the invoice-filter-ui branch May 19, 2023 04: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.

Improve invoice filtering UI
4 participants