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

Add store sort #1861

Merged
merged 1 commit into from
Aug 27, 2020
Merged

Add store sort #1861

merged 1 commit into from
Aug 27, 2020

Conversation

bolatovumar
Copy link
Contributor

Aug-22-2020 15-32-26

close #1837

@dennisreimann
Copy link
Member

Nice.

I'm having a bit of trouble with the icon though:

  • it's too bold
  • ideally it should be displayed all the time to hint that there's sorting functionality

What about something like the font awesome sort icon, but with state display: initially/unsorted grayed out arrows, sorted direction highlighted.

@bolatovumar
Copy link
Contributor Author

Nice.

I'm having a bit of trouble with the icon though:

  • it's too bold
  • ideally it should be displayed all the time to hint that there's sorting functionality

What about something like the font awesome sort icon, but with state display: initially/unsorted grayed out arrows, sorted direction highlighted.

Yeah, I can update the icon. We already use this icon on another page (apps) so I just used the same icon here.

@bolatovumar
Copy link
Contributor Author

@dennisreimann Looks like I can't use the dualtone icon to highlight the sort direction as it's only available in the pro version of Font Awesome. How about this? I added a regular sort icon for non-sorted state and an alphanumeric ascending/descending icon for the sorted states.

Aug-23-2020 16-08-01

@dennisreimann
Copy link
Member

Pinging @dstrukt here, because he might also have input :)

@dstrukt
Copy link
Member

dstrukt commented Aug 24, 2020

I do :)

  • Agree with the original sentiment, icon was way too large.
  • I would only show the sort icon on column that it's currently affecting, keeps it cleaner and easier to parse with a glance.
  • I would also recommended the two-tone, it's the most widely used and recognizable pattern.

But given the FA pro constraint...I had more questions and clarifications, which will also be helpful for the design system documentation:

  1. Are we leveraging custom icons anywhere in the project or is it all based on the basic font-awesome package?
  2. If not, was there interest in implementing the ability to support custom icons / utilizing icomoon / fontello, etc...? As I know I'll have custom icons, etc.. for us in the near future.

Assuming nothing custom, etc.. is possible, I think utilizing the A-Z could also work, however the left spacing on the icon should be adjusted slightly (2 or 4px).

Might be more than needed at this moment, but I've also mocked up a minimal state functionality for tables as well -- trying to get us to this point with all assets / components, and potentially with animatics in the future

@dennisreimann will address the invoice ticket when i get another spare chunk of tonight

TableStates

And to give further context to see an A/B/C comparison:

TableOptions

@bolatovumar
Copy link
Contributor Author

  1. Are we leveraging custom icons anywhere in the project or is it all based on the basic font-awesome package?

I believe it's mostly just the standard font-awesome icons. There are SVGs used for logos and whatnot.

  1. If not, was there interest in implementing the ability to support custom icons / utilizing icomoon / fontello, etc...? As I know I'll have custom icons, etc.. for us in the near future.

I don't see why not. As long as it's not a PITA to use from the development perspective.

Assuming nothing custom, etc.. is possible, I think utilizing the A-Z could also work, however the left spacing on the icon should be adjusted slightly (2 or 4px).

I can do this.

  • I would only show the sort icon on column that it's currently affecting, keeps it cleaner and easier to parse with a glance.

Do you mean that there should be no icon shown initially and show it only once you click on the table heading to initiate the sort? This is in conflict of the original feedback from @dennisreimann (if I understand everything correctly):

ideally it should be displayed all the time to hint that there's sorting functionality

Once we have the above questions answered I can go ahead with the changes and also update the other page which has sorting (apps page).

@dstrukt
Copy link
Member

dstrukt commented Aug 25, 2020

There's always at least one sort active (which isn't completely clear in that first mock -- should have shown it switching, instead of non-existent), but only one of the sort options should be visible at any given time.

Corrected (my apologies):

Screen Shot 2020-08-24 at 7 54 12 PM

@dennisreimann Curious to hear your thinking as well.

@bolatovumar I've used icomoon in the past and it was no different than incorporating FA into the project - but I would also lean on a/the developer to give me their final say (if they don't mind) - creating and delivering the icons is easy.

@dennisreimann
Copy link
Member

dennisreimann commented Aug 25, 2020

I like the version @dstrukt proposed, though I could see the need to display the arrows even for unsorted columns so that people know they can sort them.

For now I think we should solve this with the tools we have currently and introduce custom icons or update them during the Bootstrap 5 migration.

@bolatovumar
Copy link
Contributor Author

There's always at least one sort active (which isn't completely clear in that first mock -- should have shown it switching, instead of non-existent), but only one of the sort options should be visible at any given time.

Corrected (my apologies):

Screen Shot 2020-08-24 at 7 54 12 PM

@dennisreimann Curious to hear your thinking as well.

@bolatovumar I've used icomoon in the past and it was no different than incorporating FA into the project - but I would also lean on a/the developer to give me their final say (if they don't mind) - creating and delivering the icons is easy.

There is a bit of an issue with having the table headers be grey AND not have the sorting icon shown by default because then that looks no different from any other table which doesn't have sorting. With this style of headings users would not know that the table is sortable unless they click on the heading.

For now, I think I will leave it at this version and we can update the styling if needed after the Bootstap 5 migration that @dennisreimann mentioned above.

@bolatovumar
Copy link
Contributor Author

Updated both pages which have table sorting to have the same look:

Screen Shot 2020-08-25 at 7 27 03 PM

Screen Shot 2020-08-25 at 7 27 21 PM

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

@pavlenex pavlenex added this to Review complete in 0.09 Miles-stone Aug 26, 2020
@NicolasDorier NicolasDorier merged commit b71eb12 into btcpayserver:master Aug 27, 2020
0.09 Miles-stone automation moved this from Review complete to Done Aug 27, 2020
@bolatovumar bolatovumar deleted the feat/1837 branch September 20, 2020 00:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Default to Alphabetic Ordering of Stores List
5 participants