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

Update Bitcoin currency to use proper symbol, add mBTC and μBTC #24182

Merged
merged 7 commits into from
Jun 11, 2020
Merged

Update Bitcoin currency to use proper symbol, add mBTC and μBTC #24182

merged 7 commits into from
Jun 11, 2020

Conversation

overcookedpanda
Copy link
Contributor

This is an update to the changes I submitted in #13125
Based on #13125 (comment) and #8564 :)

@aknuds1 aknuds1 added area/frontend pr/external This PR is from external contributor labels May 2, 2020
@stale
Copy link

stale bot commented May 16, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale Issue with no recent activity label May 16, 2020
@overcookedpanda
Copy link
Contributor Author

Please don't mark as stale :)

@stale stale bot removed the stale Issue with no recent activity label May 16, 2020
@phemmer
Copy link
Contributor

phemmer commented May 20, 2020 •
edited
Loading

While this may be the more accurate representation of the symbol, it does not render well on many systems. For example mine. While the old symbol is fine.
image

IIRC, this discussion came up when the currency was first proposed several years ago.

@overcookedpanda
Copy link
Contributor Author

Ah, I also believe that's why I used the current symbol in the first PR I submitted, but it seemed to render properly on the commit, however, I did notice the same issue inside my IDE when I was making this, but figured it would render properly in modern browsers.

image

@aknuds1 aknuds1 requested review from a team, mckn and peterholmberg and removed request for a team May 21, 2020 11:48
mckn
mckn requested changes Jun 3, 2020
View reviewed changes
Copy link
Contributor

@mckn mckn left a comment

Choose a reason for hiding this comment

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

Love the idea to add these to the code base but looks like you have mixed the units together. Please update the units to reflect this list: https://en.bitcoinwiki.org/wiki/Units

@overcookedpanda @mckn
Apply suggestions from code review
150e82b 
Co-authored-by: Marcus Andersson <systemvetaren@gmail.com>
@overcookedpanda overcookedpanda requested a review from a team June 3, 2020 12:19
mckn
mckn approved these changes Jun 3, 2020
View reviewed changes
Copy link
Contributor

@mckn mckn left a comment

Choose a reason for hiding this comment

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

Great work! LGTM. Thanks for adding this @overcookedpanda 🙏

@mckn mckn added this to the 7.0.3 milestone Jun 3, 2020
@mckn mckn removed this from the 7.0.3 milestone Jun 3, 2020
@mckn
Copy link
Contributor

mckn commented Jun 3, 2020

@overcookedpanda can you latest master into this branch so it can be merged?

@mckn
Copy link
Contributor

mckn commented Jun 5, 2020

@torkelo @dprokop what to you guys think regarding the bitcoin sign?

@torkelo
Copy link
Member

torkelo commented Jun 8, 2020

Think we will have to wait a while for this one as it looks wrong for me as well (Chrome 81, Ubuntu 18)

@torkelo torkelo closed this Jun 8, 2020
@mckn
Copy link
Contributor

mckn commented Jun 8, 2020

Think we will have to wait a while for this one as it looks wrong for me as well (Chrome 81, Ubuntu 18)

what do you think about revering the bitcoin sign but merge the milli and micro BTC? @torkelo

@torkelo
Copy link
Member

torkelo commented Jun 8, 2020

that will work

@mckn mckn reopened this Jun 10, 2020
@mckn
Copy link
Contributor

mckn commented Jun 10, 2020

Think we will have to wait a while for this one as it looks wrong for me as well (Chrome 81, Ubuntu 18)

what do you think about revering the bitcoin sign but merge the milli and micro BTC? @torkelo

@overcookedpanda what do you think?

@overcookedpanda
Copy link
Contributor Author

Think we will have to wait a while for this one as it looks wrong for me as well (Chrome 81, Ubuntu 18)

what do you think about revering the bitcoin sign but merge the milli and micro BTC? @torkelo

@overcookedpanda what do you think?

Sure, I think its reasonable until the BTC symbol can be viewed on all platforms. I can update this PR with only those changes?

@mckn
Copy link
Contributor

mckn commented Jun 10, 2020

Think we will have to wait a while for this one as it looks wrong for me as well (Chrome 81, Ubuntu 18)

what do you think about revering the bitcoin sign but merge the milli and micro BTC? @torkelo

@overcookedpanda what do you think?

Sure, I think its reasonable until the BTC symbol can be viewed on all platforms. I can update this PR with only those changes?

That would be perfect! :)

@overcookedpanda
Copy link
Contributor Author

Think we will have to wait a while for this one as it looks wrong for me as well (Chrome 81, Ubuntu 18)

what do you think about revering the bitcoin sign but merge the milli and micro BTC? @torkelo

@overcookedpanda what do you think?

Sure, I think its reasonable until the BTC symbol can be viewed on all platforms. I can update this PR with only those changes?

That would be perfect! :)

Should be good to go now!

@mckn mckn merged commit d3c57ac into grafana:master Jun 11, 2020
torkelo pushed a commit that referenced this pull request Jun 24, 2020
@overcookedpanda @torkelo
Units: Added bitcoin units milli-bitcoin (mBTC) and micro-bitcoin (μB…
5147b1a 
…TC) (#24182)

* Update Bitcoin Currency to use proper symbol, add mBTC and μBTC

* Apply suggestions from code review

Co-authored-by: Marcus Andersson <systemvetaren@gmail.com>

* Revert Bitcoin to use '฿'

Co-authored-by: Marcus Andersson <systemvetaren@gmail.com>
(cherry picked from commit d3c57ac)
@torkelo torkelo mentioned this pull request Jun 24, 2020
Merged
3 tasks
@torkelo torkelo modified the milestones: 7.1, 7.0.4 Jun 24, 2020
dprokop pushed a commit that referenced this pull request Jun 25, 2020
@overcookedpanda @dprokop
Units: Added bitcoin units milli-bitcoin (mBTC) and micro-bitcoin (μB…
963e3ba 
…TC) (#24182)

* Update Bitcoin Currency to use proper symbol, add mBTC and μBTC

* Apply suggestions from code review

Co-authored-by: Marcus Andersson <systemvetaren@gmail.com>

* Revert Bitcoin to use '฿'

Co-authored-by: Marcus Andersson <systemvetaren@gmail.com>
(cherry picked from commit d3c57ac)
@dprokop dprokop mentioned this pull request Jun 25, 2020
Merged
aknuds1 pushed a commit that referenced this pull request Jun 30, 2020
@overcookedpanda @mckn @aknuds1
Units: Added bitcoin units milli-bitcoin (mBTC) and micro-bitcoin (μB…
65df313 
…TC) (#24182)

* Update Bitcoin Currency to use proper symbol, add mBTC and μBTC

* Apply suggestions from code review

Co-authored-by: Marcus Andersson <systemvetaren@gmail.com>

* Revert Bitcoin to use '฿'

Co-authored-by: Marcus Andersson <systemvetaren@gmail.com>
@phemmer
Copy link
Contributor

phemmer commented Jul 6, 2020

Was doing some unicode stuff when I stumbled across the ₿ symbol. I found it odd that my browser was rendering this properly considering the recent discussion on this PR. But it appears that this is a completely different symbol than what was proposed. In fact, it seems this symbol is the "official" symbol as well (however official a decentralized currency with no owner can be), at least according to wikipedia.

Anyway, just mentioning in case you wanted to revive the issue.

@mckn
Copy link
Contributor

mckn commented Jul 7, 2020

Was doing some unicode stuff when I stumbled across the ₿ symbol. I found it odd that my browser was rendering this properly considering the recent discussion on this PR. But it appears that this is a completely different symbol than what was proposed. In fact, it seems this symbol is the "official" symbol as well (however official a decentralized currency with no owner can be), at least according to wikipedia.

Anyway, just mentioning in case you wanted to revive the issue.

Interesting, maybe something worth opening a new PR on? Not sure how much this is used and how big of an issue it is.

@overcookedpanda
Copy link
Contributor Author

Was doing some unicode stuff when I stumbled across the ₿ symbol. I found it odd that my browser was rendering this properly considering the recent discussion on this PR. But it appears that this is a completely different symbol than what was proposed. In fact, it seems this symbol is the "official" symbol as well (however official a decentralized currency with no owner can be), at least according to wikipedia.

Anyway, just mentioning in case you wanted to revive the issue.

I confirmed that this symbol works with Brave on Linux which did not work last time, however, it seems iOS has some issue with this one, lol.

In the Mail app's preview I see the following, which looks good:
image

However, once I open the PR and view I see that it does not render correctly:
image

I would think it would be best to keep using the Thai Baht symbol for the time being, and just check for support later and re-visit the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mckn mckn mckn approved these changes

@peterholmberg peterholmberg Awaiting requested review from peterholmberg

Assignees
No one assigned
Labels
add to changelog area/frontend effort/small pr/external This PR is from external contributor
Projects
None yet
Milestone
7.0.4
Development

Successfully merging this pull request may close these issues.
6 participants
@overcookedpanda @phemmer @mckn @torkelo @aknuds1 @dprokop