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

Web view improvements (update interval, settings) #286

Merged
merged 27 commits into from
Apr 16, 2017

Conversation

ckcollab
Copy link
Contributor

@ckcollab ckcollab commented Mar 31, 2017

Todo

  • Save settings to localStorage
  • Read settings from localStorage
  • Modal for settings
  • Gear icon
  • Add "years" to time views
  • Limit USD view to only show 2 decimal places Believe this is being handed off to Total Account Holdings feature?

Description

This makes the page update every 10 seconds instead of every 30.

TESTING STAGE

Currently testing this for 24hrs

Types of changes

  • [~] New feature (non-breaking change which adds functionality)

Checklist:

  • I have read CONTRIBUTING.md
  • I fully understand Github Flow.
  • My code adheres to the code style of this project.
  • I have updated the documentation in /docs if I have changed the config, arguments, logic in how the bot works, or anything that understandably needs a documentation change.
  • I have updated the config file accordingly if my change requires a new configuration setting or changes an existing one.
  • I have tested the bot with no issues for 24 continuous hours. If issues were experienced, they have been patched and tested again.

@Evanito Evanito added the Queued label Mar 31, 2017
@rnevet
Copy link
Collaborator

rnevet commented Mar 31, 2017

Why? The bot has a slow cycle.
How fast are you running the bot?

Since my bot is running at 1 min cycle, 30 sec is optimal for getting reasonable updates and not wasting bandwidth on mobile.

In any case, please make it configurable, I see 2 options:

  1. Half of the bot sleep time. (The value needs to be passed in the json)
  2. Using a URL parameter.

@ckcollab
Copy link
Contributor Author

Interval ~10seconds -- I love watching the log flying around right after I make changes, etc. across all my coins (and their prices). I'm used to streaming output so I like it as up-to-date as possible :)

I mean, ideally it should be configurable based on preferences. I could add a button/slider/drop down to set this value?

@rnevet
Copy link
Collaborator

rnevet commented Mar 31, 2017

We have already some URL options.
It would be great to get a "settings" area if you can, otherwise another URL parameter should do it.

@ckcollab
Copy link
Contributor Author

Oh. I wasn't even aware of the URL options! I skimmed over that in the docs.

...maybe I can add a nice little section for managing all of these options... :)

For my reference http://poloniexlendingbot.readthedocs.io/en/latest/configuration.html#lendingbot-html-options

@rnevet
Copy link
Collaborator

rnevet commented Mar 31, 2017

That would be great, never got to that.
Keep in mind that the page is mobile friendly.

@rnevet
Copy link
Collaborator

rnevet commented Apr 1, 2017

Just use prettyFloat instead of printFloat.

@rnevet
Copy link
Collaborator

rnevet commented Apr 2, 2017

Actually I would like the time views to be selectable by checkboxes in the settings as well, personally I would look at daily, monthly only.

@rnevet
Copy link
Collaborator

rnevet commented Apr 3, 2017

btw, I was working here to add "Total Account Value":
#246

@@ -246,6 +258,8 @@ function Timespan(name, multiplier) {
}
if (currency == "BTC") {
return displayUnit.formatValue(earnings) + " <span class=" + currencyClass + ">" + displayUnit.name + "</span> / " + name + "<br/>"
} else if (currency == "USD" || currency == "USDT") {
return printFloat(earnings, 2) + " <span class=" + currencyClass + ">" + currency + "</span> / "+ name + "<br/>";
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest we just use prettyFloat(earnings,2) instead of trying to guess which currency is a Fiat one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

i.e. I use EUR. ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would change behavior for quite a few coins, I think we want to see Satoshi level resolution most of the times?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What other currencies should we add here? Some currencies may need special printing, in this case for USD it makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, the other currencies in this function use printFloat, you want to go in and change these to the style you prefer?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ckcollab "EUR" for example and actually any currency supported by https://blockchain.info/api/exchange_rates_api

I'm thinking about using the prettyFloat as it will provide an accuracy of 2 decimals for any coin and I think that should be good enough, no? or should we make the accuracy configurable?

I'm open for suggestions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggestion: I also add EUR, but maintain consistency with the surrounding code. Then in a separate PR we decide how to handle this particular problem.

Copy link
Collaborator

Choose a reason for hiding this comment

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

agreed.

total_cell.innerHTML = "Total holdings";
total_cell = total_row.appendChild(document.createElement("th"));
total_cell.setAttribute("colspan", 2);
total_cell.innerHTML = prettyFloat(totalCoinsOverall, 2) + " USD";
Copy link
Collaborator

@rnevet rnevet Apr 5, 2017

Choose a reason for hiding this comment

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

USD? I guess it should be earningsOutputCoin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since you're doing this in another PR I'll pull this stuff

@rnevet
Copy link
Collaborator

rnevet commented Apr 5, 2017

If you need a Gear icon you can use one from http://fontawesome.io/icons/

@ckcollab ckcollab changed the title Increase page update interval Web view improvements (interval, settings) Apr 6, 2017
@ckcollab ckcollab changed the title Web view improvements (interval, settings) Web view improvements (update interval, settings) Apr 6, 2017
@ckcollab
Copy link
Contributor Author

ckcollab commented Apr 6, 2017

If you need a Gear icon you can use one from http://fontawesome.io/icons/

Just using the Bootstrap one, is fontawesome also included? Probably not necessary to have two icon libs when we only have the 1 page.

@ckcollab
Copy link
Contributor Author

Gear icon looks out of place and a little lopsided.

Goahead and replace, I don't mind :)

Entering out of bounds values for refresh time are "saved" when an error should be shown.

Added a check for this.

Would you be willing to move the other web settings to this settings box?

I'd rather not, very very busy! Glad to share what little I could given I use this all the time, thanks for all the hard work!

@rnevet
Copy link
Collaborator

rnevet commented Apr 14, 2017

It is great! We will build upon. :)

@Evanito
Copy link
Member

Evanito commented Apr 15, 2017

I move to merge in its current state, since I don't want to muck around with multiple branches while trying to change a single icon.
Thank you for your contribution!

@rnevet I leave final decision to you.

@rnevet
Copy link
Collaborator

rnevet commented Apr 15, 2017

@Evanito I've made the rest of the changes and minor fixes in this branch https://github.com/Mikadily/poloniexlendingbot/tree/web_setting. Made a PR back to @ckcollab and when he merges it, it should appear here. (Otherwise we can create a new PR from web_setting branch.)

rnevet and others added 5 commits April 15, 2017 13:21
Replace printFloat with prettyfloat for Fiat currencies (doesn't display .00)
…o get bad data set (sets localStorage without checking if it's valid)
As btcDisplayUnitsModes isn't a list of strings a simple indexOf won't match the mode.
new header with collapsing menu
@rnevet
Copy link
Collaborator

rnevet commented Apr 16, 2017

@yura-pakhuchiy can you test this as well?
@Evanito can you take a 2nd look?

@yura-pakhuchiy
Copy link
Contributor

LGTM! I had exactly the same idea for settings dialog on webpage itself for quite some time. I have never found time to implement it. @ckcollab @rnevet Greak work!

Small things which can be improved:

  1. Use 192 icon in navbar. It will look sharper on mobiles or if user zooms in on desktop.
  2. Auto-collapse navbar after user clicked on settings. It is annoying a bit that navbar is still open after user closes settings dialog.

@rnevet rnevet merged commit c50f27b into BitBotFactory:master Apr 16, 2017
@rnevet
Copy link
Collaborator

rnevet commented Apr 16, 2017

@ckcollab Thanks!

@ckcollab
Copy link
Contributor Author

@rnevet my pleasure, thanks for this project!

@laxdog
Copy link
Collaborator

laxdog commented May 3, 2017

Just saw these changes as I've been working on an old branch. They look really great. Nice work @ckcollab and @rnevet

@ckcollab
Copy link
Contributor Author

ckcollab commented May 3, 2017

Least I could do :)

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.

5 participants