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.

Sign up for GitHub

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

Jump to bottom

Fixes for issues 2944 and 2955 #2972

Closed
wants to merge 5 commits into from
Closed

Fixes for issues 2944 and 2955 #2972

wants to merge 5 commits into from

Conversation

niyid
Copy link
Contributor

@niyid niyid commented Jul 18, 2019

  1. Issue 2944 is a fix for a bug where the low and high statistics are flipped when the currency is a cryptocurrency.
  2. Issue 2955 required adding a median computation as part of the statistics. This is a fairly straightforward feature add.

niyid added 4 commits July 18, 2019 13:56
@niyid
Update displayStrings.properties
51cdc83 
Adding new label for median.
@niyid
Update CandleTooltip.java
8094ddc 
Add median statistics element.
@niyid
Update CandleData.java
8ee165a 
Added median statistics element.
@niyid
Update TradesChartsViewModel.java
90e8552 
Added median statistics element.
@niyid niyid requested review from ripcurlx and sqrrm as code owners July 18, 2019 13:06
@niyid
Update TradesChartsViewModelTest.java
70eddde 
Added median statistic as part of elements to test.
sqrrm
sqrrm requested changes Jul 18, 2019
View reviewed changes
Copy link
Member

@sqrrm sqrrm 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 your IDE might be messing up the files you touch. Both the formatting and some characters. Please take care to setup your environment correctly. There is a guide to get the formatting right. The issue with the characters I don't know about.

The actual fix might be correct but it's very hard to review among the mess.

@niyid
Copy link
Contributor Author

niyid commented Jul 18, 2019 •
edited
Loading

I think your IDE might be messing up the files you touch. Both the formatting and some characters. Please take care to setup your environment correctly. There is a guide to get the formatting right. The issue with the characters I don't know about.

The actual fix might be correct but it's very hard to review among the mess.

OK. I will reformat and check in. Is there any way I can see what you have? My compare says they are the same with my local.

@sqrrm
Copy link
Member

sqrrm commented Jul 18, 2019

Take a look under files changed tab here in the PR, https://github.com/bisq-network/bisq/pull/2972/files

If you have no diff locally your system must be changing stuff on its own. What OS are you on?

@niyid
Copy link
Contributor Author

niyid commented Jul 18, 2019

Take a look under files changed tab here in the PR, https://github.com/bisq-network/bisq/pull/2972/files

If you have no diff locally your system must be changing stuff on its own. What OS are you on?

Do you refer to the contents of displayStrings.properties? I noticed that even on my local since I first pulled. I am using Fedora 22.

@niyid niyid closed this Jul 18, 2019
@sqrrm
Copy link
Member

sqrrm commented Jul 18, 2019

Both displayStrings.properties and TradesChartsViewModelTest.java in which you seem to have put TradesChartsViewModel, and formatted with 8 space tabs, should be 4 spaces.

@niyid
Copy link
Contributor Author

niyid commented Jul 18, 2019

Apologies on the mistakes. I forked again and recommitted. Once again sorry for any inconvenience.

@sqrrm sqrrm mentioned this pull request Aug 7, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sqrrm sqrrm sqrrm requested changes

@ripcurlx ripcurlx Awaiting requested review from ripcurlx

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@niyid @sqrrm