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

Adjust numbers alignment #4171

Closed

Conversation

freimair
Copy link
Member

Fixes #3991

This PR fixes the most visible number alignment issues. Omitted the stuff in [Funds] and in other places, because there would be more to it than just pushing around css classes.

Here are some impressions:
Screenshot from 2020-04-14 20-31-40
Screenshot from 2020-04-14 20-31-27
Screenshot from 2020-04-14 20-31-13

@ripcurlx
Copy link
Member

The problem I see with that is that @pedromvpg wanted to have the columns aligned in the same way except for the last one. To make this possible I implemented a solution that works with monospace fonts. As this was dropped because of font style issues, the alignment doesn't work properly anymore.

@freimair
Copy link
Member Author

just to check if I understood correctly:

  • you ditched Monospace font because of aesthetic reasons (so no alignment anymore)
  • we do not want aligned comma points because of @pedromvpg and column alignment

I understand that @pedromvpg design (with monospace) had the numbers aligned by comma. So he did design it that way didn't he? Only after ditching the monospace font, the numbers became misaligned.

Given the great work @pedromvpg did with designing the UI, I somehow doubt that he would consider having number not aligned by its comma as aesthetic. Especially in an application where the numbers are its heart and soul.

@stale
Copy link

stale bot commented Jun 5, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot unassigned pedromvpg Jun 5, 2020
@stale stale bot added the was:dropped label Jun 5, 2020
@freimair
Copy link
Member Author

still relevant

@stale stale bot removed the was:dropped label Jun 10, 2020
@sqrrm sqrrm requested a review from ripcurlx July 1, 2020 09:54
@cd2357
Copy link
Contributor

cd2357 commented Jul 28, 2020

ACK

I tested it and and indeed, numbers are much more readable when they're right-aligned.

Looks like a simple and elegant solution (using CSS classes), I'd say if other ongoing UI work "conflicts" with this, it would be trivial to just rebase and merge, or somehow integrate it if necessary.

So I'd say this PR it's worth merging.

@stale
Copy link

stale bot commented Aug 28, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the was:dropped label Aug 28, 2020
@freimair
Copy link
Member Author

freimair commented Sep 1, 2020

I still think this is relevant. @ripcurlx would you assign @pedromvpg for review or merge it right away?

@stale stale bot removed the was:dropped label Sep 1, 2020
@ripcurlx
Copy link
Member

ripcurlx commented Sep 3, 2020

I still think this is relevant. @ripcurlx would you assign @pedromvpg for review or merge it right away?

Unfortunately I'm not able to assign him as reviewer. Will ping him on keybase.

@stale
Copy link

stale bot commented Oct 4, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the was:dropped label Oct 4, 2020
@cd2357
Copy link
Contributor

cd2357 commented Oct 4, 2020

Still relevant (even now, 6 months after PR was created).

Can someone with a better understanding of this shortly indicate what is holding it back? Is it:

a) this PR is fine, but needs final approval from @pedromvpg
b) this PR is fine, just needs to resolve conflicts with the latest master (since its not clear if / when @pedromvpg will have time to review)
c) this PR needs some adjustments first (see comments above re: last column)
d) this PR has to wait, cause other UI changes are coming soon (new design?) and merging this would complicate the upcomnig UI efforts
e) smth else

@stale stale bot removed the was:dropped label Oct 4, 2020
@pedromvpg
Copy link
Member

Hey,

I won't be able to compile until later tonight.
Can you post a before and after screenshot to compare the changes?

Thanks

@ripcurlx
Copy link
Member

ripcurlx commented Oct 6, 2020

Hey,

I won't be able to compile until later tonight.
Can you post a before and after screenshot to compare the changes?

Thanks

Hi @pedromvpg! Please see #4171 (comment) . @freimair basically changed the alignment for all number columns to right-align instead of left-align.

@pedromvpg
Copy link
Member

I think keeping all of the columns left aligned and the last column centered if its a column with check boxes would be the easiest to read.

I'm of the preference of keeping the 'Amount' column alignment consistent with the rest more than keeping the commas aligned.

image

image

@pedromvpg
Copy link
Member

@freimair
My priority is to keep the overall presentation clean. By aligning the comas, mostly on the amount column, you think would increase the ease of legibility of the numerals without hurting the legibility of the overall table?

In my view it helps in the specifics but it really makes the table look messy.

@cd2357
Copy link
Contributor

cd2357 commented Oct 7, 2020

I'm of the preference of keeping the 'Amount' column alignment consistent with the rest more than keeping the commas aligned.

I agree with @freimair on this, right-aligning for numeric values makes them more readable because of the comma alignment. Bitfinex and Kraken dashboards right-align them as well.

The material design guidelines also default to text as left-aligned, numbers as right-aligned.

..the last column centered if its a column with check boxes..

Here I agree with @pedromvpg , checkbox columns look better when center-aligned.

Also text columns should probably stay left-aligned, as @pedromvpg suggested (for consistency with the rest of the UI). This PR, in the current form, also changes this (for example, Offer ID becomes right-aligned, even though its not a numeric value).


From what I can tell, the main point of this PR was the numeric alignment. So to simplify the decision, let's just concentrate on this one aspect (and leave all else unchanged, like text left-aligned, checkboxes center-aligned).

It seems there are strong pros and cons for left and right aligning numbers.

Maybe @m52go could run a user survey on twitter, and ask them to vote between

A) left-aligned number columns

Screenshot 2020-10-07 at 12 24 00

B) right-aligned number columns

Screenshot 2020-10-07 at 12 22 22

Or maybe there's another, better way to get user feedback?

Would this make sense?

@m52go
Copy link
Contributor

m52go commented Oct 7, 2020

It's not clear to me what's best here so I'd personally lean toward whatever Pedro recommends but...might not hurt to ask users too.

https://twitter.com/bisq_network/status/1313859755975176193

@m52go
Copy link
Contributor

m52go commented Oct 7, 2020

Well it seems the pro designers favor left-align (Pedro and this guy) while everyone else favors right-align.

@stale
Copy link

stale bot commented Nov 7, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the was:dropped label Nov 7, 2020
@stale
Copy link

stale bot commented Dec 12, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale
Copy link

stale bot commented Jan 17, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the was:dropped label Jan 17, 2021
@stale
Copy link

stale bot commented Jun 11, 2021

This issue has been automatically closed because of inactivity. Feel free to reopen it if you think it is still relevant.

@stale stale bot closed this Jun 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix numbers alignment in GUI
5 participants