-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Localize timeseries tooltips #2648
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
Conversation
|
LGTM, thx |
|
@yuzawa-san no monospaced font, it causes crashes on some macs |
|
i updated it. do you know which version(s) it is an issue? i think this app could benefit nicely from monospaced digit system font being used more widely especially given how many digits we display. would it be worthwhile to add (not in this PR) some sort of polyfill like this i can make an issue and/or give it a try if you want |
|
no monospaced font. I was ready to merge that, but I have noticed that you make a lot of unnecessary changes. What is for? It's a PR for localize timeseries tooltips, so please make sure it's only one change. |
5949d0f to
0007350
Compare
|
reverted back to the initial commit you were ready to approve |
| let date = self.dateFormatter.string(from: nearest.value.ts) | ||
| let subtitleWidth = date.widthOfString(usingFont: subtitleFont) | ||
| let width = max(70, subtitleWidth) + 8 | ||
| let roundedValue = (nearest.value.value * 100).rounded(toPlaces: 2) | ||
| let strValue = roundedValue >= 1 ? "\(Int(roundedValue))\(suffix)" : "\(roundedValue)\(suffix)" | ||
| let value = toolTipFunc != nil ? toolTipFunc!(nearest.value) : strValue | ||
| drawToolTip(self.frame, CGPoint(x: nearest.point.x+4, y: nearest.point.y+4), CGSize(width: 78, height: height), value: value, subtitle: date) | ||
| drawToolTip(self.frame, CGPoint(x: nearest.point.x+4, y: nearest.point.y+4), CGSize(width: width, height: height), value: value, subtitle: date) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how this change is related to the date format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is because not every locale has the same format and so different locales have different width resulting strings. if i did not add this, some locales would have their strings truncated based on the existing width.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there was a reason why I do that with static width: with dynamic there was too many resizes and tooltip box "jumps" all the time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ha thats why i tried the monospaced font, before you flagged that as not working. once again it would really awesome to figure out what version that is broken in and gracefully fall back, but that is out of the scope of this PR. i just retested this. here is a video in my locale. typically only a single digit is changing. is this acceptable?
Screen.Recording.2025-08-31.at.16.07.14.mov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, I really do not like jumping boxes. If you want to get more info about monospaced font just search in issues about that, there was a few times when I use that font.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given the variance is within a few pixels in the absence of a monospace font. can i just calculate the width for the locale in the constructor add like a few pixels of buffer and then reuse that width in the render to prevent the jitter?
|
this is all becoming too much trouble, so i propose #2689 instead |
I found the tooltips were not localized, so I formatted the date/time in the user's locale.
example: for locale en_US
