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

Cleaner formatting for trading charts date axes #4715

Merged
merged 3 commits into from
Nov 2, 2020
Merged

Cleaner formatting for trading charts date axes #4715

merged 3 commits into from
Nov 2, 2020

Conversation

deusmax
Copy link
Contributor

@deusmax deusmax commented Oct 27, 2020

Clean up date axes style for easy reading. Made date labels shorter,
with clear visual correspondence to relevant axes tick mark.
Tick marks with labels are now larger than those without.

Currently, the X-axis for the Price and Volume charts is not well designed. The labels are large, all the tick marks are the same. It is difficult to estimate to which tick mark the date label corresponds. Here is a typical example for the DAY interval:
Bisq-chartaxes-day_orig-2020-10-27

The fix proposed here draws the labels more compact and the respective ticks stand-out. The chart view becomes cleaner and more visually intuitive. Above example, becomes:
Bisq-chartaxes-day_new-2020-10-27

The labels take less horizontal space by splitting into 2-levels, using 24-hour time instead of AM/PM. The examples here show English locale, though localization for the month names is maintained, as before.

Of course, further customization is possible, but the above, I feel, provides an acceptable improvement to chart display, without deviating from the current UI or introducing new features.

Similar results are obtained with the other chart interval values. Examples (10min, HOUR, WEEK, MOTH):
Bisq-chartaxes-10min_new-2020-10-27
Bisq-chartaxes-hour_new-2020-10-27
Bisq-chartaxes-week_new-2020-10-27
Bisq-chartaxes-month_new-2020-10-27

Bisq is a unique tool. It deserves better charts.
Hope you find this a small improvement in the right direction.

@boring-cyborg
Copy link

boring-cyborg bot commented Oct 27, 2020

Thanks for opening this pull request!

Please check out our contributor checklist and check if Travis or Codacy found any issues with your PR. Also make sure your commits are signed, and that you applied Bisq's code style and formatting.

A maintainer will add an is:priority label to your PR if it is up for compensation. Please see our Bisq Q1 2020 Update post for more details.

Clean up date axes style for easy reading. Made date labels shorter,
with clear visual correspondance to relevant axes tick mark.
Tick marks with labels are now larger than those without.

Signed-off-by: Deus Max <[email protected]>
@ripcurlx
Copy link
Contributor

@deusmax I think this is a good idea! Maybe we can get it even cleaner by getting rid of the day & month when year is selected.
Bildschirmfoto 2020-10-29 um 10 41 27
And get rid of the day when month is selected.
Bildschirmfoto 2020-10-29 um 10 41 12
What do you think?

@deusmax
Copy link
Contributor Author

deusmax commented Oct 29, 2020

@ripcurlx Thanks. Agree these are good points and indeed the charts would be even better.
These changes you propose, may require to do some minor work on the chart logic.
The code here applies changes only the chart's element configuration/formatting. It does not make any logical changes, even minor ones. In fact I explicitly avoided doing so.

I'll start working on the labels as you point out.... and get back.
Also, the Year-interval charts don't really need to go all the way back to 1930 !!, do they ?

Provide specific formats for each trade chart interval period, for added
precision and flexibility in formatting the X-axis. Covers suggestion in
Bisq PR#4715 for more compact date formats.
@deusmax
Copy link
Contributor Author

deusmax commented Oct 29, 2020

Implemented the suggestions by @ripcurlx .

Yearly charts, show only year.
Bisq-chartaxes-year_new-2020-10-29

Monthly charts, got rid of the date and chopped the year.
Bisq-chartaxes-month_new-2020-10-29

Copy link
Contributor

@ghubstan ghubstan left a comment

Choose a reason for hiding this comment

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

I tested it, looks nice. If I could (I'm not a JFX expert), I would center-align the labels where possible, e.g.,

29/Oct
 2020

But I would not block approval and merging without that.
I will add a Tested ACK if the switch statement styling is changed.

case HOUR :
case MINUTE_10: fmt = "HH:mm\ndd/MMM";
break;
default: // nothing here
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to reformat the style of the switch statement so fmt = "..." are on separate lines. Not sure codacy will complain about this, but other devs might want the switch style to be consistent with the rest of the code base.
(This is the 1st time I've seen this style in Bisq; I'm not saying there may be other examples in the code base.)

switch (model.tickUnit) {
                    case YEAR:
                        fmt = "yyyy";
                        break;
                    case MONTH:
                        fmt = "MMMyy";
                        break;
                    case WEEK:
                    case DAY:
                        fmt = "dd/MMM\nyyyy";
                        break;
                    case HOUR:
                    case MINUTE_10:
                        fmt = "HH:mm\ndd/MMM";
                        break;
                    default:        // nothing here
                }

Fixed a switch statement formatting to conform with Bisq code style.
@deusmax
Copy link
Contributor Author

deusmax commented Oct 31, 2020

@ghubstan : pushed the formatting block changes.

Still looking into centering the labels.

@ghubstan
Copy link
Contributor

@deusmax:

I will add a Tested ACK if the switch statement styling is changed.

@ghubstan : pushed the formatting block changes.

Still looking into centering the labels.

Tested ACK.

@sqrrm sqrrm merged commit e082ac6 into bisq-network:master Nov 2, 2020
@boring-cyborg
Copy link

boring-cyborg bot commented Nov 2, 2020

Awesome work, congrats on your first merged pull request!

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.

4 participants