-
-
Notifications
You must be signed in to change notification settings - Fork 6k
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
[Wrong?] for #682: adjust point.y to count text height, in order to draw it in center #715
Conversation
…raw it in center
@petester42 I may need your help here. Not very familiar with the tests. Is this because I changed noDataText position so the related test cases will fail? |
Probably yes. If the images don't match exactly then they will fail. For easy comparison I use this https://github.com/orta/Snapshots |
does this mean I need to update all the related images along with this PR? |
If you believe that the resulting images are correct then ya. It's pretty easier. Set recordMode to true. Run tests. Set it back to false. |
Thanks. Will take a look at the test suite |
@liuxuan30 I'm sorry to disappoint you, but that function should not adjust the Notice that there are several text drawing functions. This one specifically is for single-line text, where aligning is done on the X axis only (hence There are other text drawing functions, where both X and Y are adjusted, according to alignment and anchoring - suited for much complex situations. Written first to allow rendering the rotated axis labels but keep them aligned according to their "outer" bounding rectangles, and then used for other multiline texts. Is there a specific issue you were trying to solve? |
@petester42 the tests are great. In other cases I would most probably have not taken a deep look at the commit, assuming it's a one-liner and intuitively seems correct... |
@danielgindi the issue is for #682, saying the no data text is not centered vertically. Do you think we could adjust Y here? ChartUtils.drawText(context: context, text: noDataText, point: CGPoint(x: frame.width / 2.0, y: frame.height / 2.0), align: .Center, attributes: [NSFontAttributeName: infoFont, NSForegroundColorAttributeName: infoTextColor]) it does not count the text height / 2. btw, looks like we need some README section for workflow of updating test cases/images if the PRs will fail the test for sure? Like test failed -- changes makes sense -- update test cases/images |
Yes but that's not where we need to adjust it. We need to offset the whole drawCenterText, but half the height of both the text and description text |
As for tests - it's documented in the tests code :-) There's a line to be set to |
for #682: adjust point.y to count text height, in order to draw it in center
Seems we forgot to calculate y or by purpose ? @danielgindi
I see
get the center point, but later it just adjust x.