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

[Bug] Fix 12350 format in tooltip #1327

Merged
merged 2 commits into from Dec 15, 2020

Conversation

b2kdaman
Copy link
Contributor

Somehow formatting for 12350 was faulty and misleading from the start

Signed-off-by: Valentine <vpanchin@gmail.com>
@b2kdaman
Copy link
Contributor Author

@macrigiuseppe @heshan0131

@heshan0131
Copy link
Contributor

heshan0131 commented Dec 1, 2020

As a best practice, can you please create an issue first before creating a PR next time? An issue can be searched in the future and more people can contribute to the discussion

I think the misleading part is the label not the format itself. I would not change the .4r, rather update the label. A format once created should behave the same for backward compatibility. So that people's saved map will show the same tooltip even when the software is updated.

the .4r does exactly what it should do: rounds to 4 significant digits so that 12345 -> 12350, let's use 12345 -> 12350 as label instead of 12350

One improvement you can make is add ~ and make it .4~r so it trims the empty 0s

0.1 -> 0.1 (instead of 0.100)

@b2kdaman
Copy link
Contributor Author

b2kdaman commented Dec 1, 2020

I would not change the .4r

OR

One improvement you can make is add ~ and make it .4~r so it trims the empty 0s

So which one is it?
.4~r would break compatibility. Seems like our "fixes" doesn't really fit common use cases @thunter009

@b2kdaman
Copy link
Contributor Author

b2kdaman commented Dec 1, 2020

As a best practice, can you please create an issue first before creating a PR next time? An issue can be searched in the future and more people can contribute to the discussion
Yes you're right. I thought that it would be small fixes that would not fire such a discussion 🤣 My bad.

@b2kdaman b2kdaman force-pushed the fixed_12350_formatting branch 2 times, most recently from e52c9f5 to 4f269d2 Compare December 8, 2020 22:29
Signed-off-by: Valentine <vpanchin@gmail.com>

updated arrow

Signed-off-by: Valentine <vpanchin@gmail.com>
@b2kdaman
Copy link
Contributor Author

b2kdaman commented Dec 9, 2020

@heshan0131 please review

@b2kdaman
Copy link
Contributor Author

@heshan0131 ?

@heshan0131 heshan0131 merged commit d6e2837 into keplergl:master Dec 15, 2020
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.

None yet

2 participants