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 price rate to browser title #1117

Merged
merged 4 commits into from
Jul 23, 2021
Merged

Conversation

vctt94
Copy link
Member

@vctt94 vctt94 commented Jun 29, 2021

closes #1053

Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

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

Looks good but doesn't seem to update unless I refresh the page, so if you are viewing a different page it doesn't change.

const { book } = mktBook
// gets first price value from buy or from sell, so we can show it on
// title.
const firstKnownValue = book.buys[0] ? book.buys[0] : book.sells[0]
Copy link
Member

Choose a reason for hiding this comment

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

With an empty book

image

@@ -959,6 +959,16 @@ export default class MarketsPage extends BasePage {
handleBookRoute (note) {
app.log('book', 'handleBookRoute:', note)
const mktBook = note.payload
const { book } = mktBook
Copy link
Member

Choose a reason for hiding this comment

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

Use const book = mktBook.book for grabbing just one property.

const { book } = mktBook
// gets first price value from buy or from sell, so we can show it on
// title.
const firstKnownValue = book.buys[0] ? book.buys[0] : book.sells[0]
Copy link
Member

Choose a reason for hiding this comment

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

You can update the title after the call to this.handleBook below and just use this.midGap() for the value.

Comment on lines 968 to 970
document.title = (document.title).replace(/\d+([.])?/g, '')
// more than 6 numbers it gets too big for the title.
document.title = `${firstKnownValue.rate.toFixed(6)} ${document.title}`
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to see the market name. Something like 0.00512 DCR/BTC.

I don't know that we need to save the original title, which said Markets | Decred DEX, but if you do want to append the original text, grab a copy in the MarketPage constructor, this.ogTitle = document.title and just append it rather than using replace.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed the replace and now I am adding the selected assets into the title.

Added a new method for updating it, and now I call it on handleBookRoute, handleUnbookOrderRoute, and handleBookOrderRoute.

This way if we adding or removing new bids, it will update the title based on it.

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to see the market name. Something like 0.00512 DCR/BTC.

Despite calling it the market name, is giving units to that rate, so it'll need to be BTC/DCR

Copy link
Member

@chappjc chappjc Jul 16, 2021

Choose a reason for hiding this comment

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

Discussion in this thread: #1117 (comment)
Looks like use "DCR-BTC" and don't have units for the number.

@@ -959,6 +959,16 @@ export default class MarketsPage extends BasePage {
handleBookRoute (note) {
Copy link
Member

Choose a reason for hiding this comment

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

Doing this in handleBookRoute will set the price when the book is originally received, but will never update the value. I'm guessing we want the value to update when the book changes.

Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

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

Looks pretty great to me.

const market = this.market
const [b, q] = [market.baseCfg, market.quoteCfg]
const firstSymbol = b.symbol.toUpperCase()
const secondSybol = q.symbol.toUpperCase()
Copy link
Member

Choose a reason for hiding this comment

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

spelling: "Sybol"
But might as well change these to baseSymb and quoteSymb while here.

const secondSybol = q.symbol.toUpperCase()
if (midGapValue) {
// more than 6 numbers it gets too big for the title.
document.title = `${midGapValue.toFixed(5)} ${firstSymbol}/${secondSybol}`
Copy link
Member

Choose a reason for hiding this comment

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

This is gonna say something like 0.004 DCR/BTC. The rate is inversed.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe use a dash instead of a slash. When naming a market, the base symbol goes first by convention.

Copy link
Member

Choose a reason for hiding this comment

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

I will note that Bittrex bucks this trend

image

probably because of how confusing it is to have it written the other way.

Still, it's true that the base asset is usually first.

Copy link
Member

@buck54321 buck54321 Jul 16, 2021

Choose a reason for hiding this comment

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

Somewhere in the Bittrex API docs, I remember reading an acknowledgement of this mistake. The URL and API arguments now use the standard format. Only the display is still wrong.

Copy link
Member

Choose a reason for hiding this comment

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

OK fair enough. I see the url as https://bittrex.com/Market/Index?MarketName=BTC-DCR, but I don't really strive to emulate Bittrex either way.

This PR is working well for me. However, here's what I see presently:

image
image

And here's what I'd like to see:

image
image

OR even without a dash, but it's not a big deal:

image

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed it and now I am showing as your last option, without dash.

Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

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

Can clean up a tiny bit, but looks great.

client/webserver/site/src/js/markets.js Show resolved Hide resolved
client/webserver/site/src/js/markets.js Outdated Show resolved Hide resolved
@chappjc chappjc merged commit 6231cf4 into decred:master Jul 23, 2021
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.

display price in browser window title
4 participants