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

Fix BalloonMarker's text position calculation, consider insets #3035

Merged
merged 1 commit into from
Jan 15, 2018

Conversation

LavareX
Copy link
Contributor

@LavareX LavareX commented Nov 23, 2017

Consider insets, adjust the position of the label in the context of the center.

@codecov-io
Copy link

codecov-io commented Nov 23, 2017

Codecov Report

Merging #3035 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3035   +/-   ##
=======================================
  Coverage   19.77%   19.77%           
=======================================
  Files         113      113           
  Lines       13663    13663           
=======================================
  Hits         2702     2702           
  Misses      10961    10961

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc40b16...ea00ed0. Read the comment docs.

@liuxuan30
Copy link
Member

please give a context what's wrong.

@liuxuan30 liuxuan30 self-requested a review December 12, 2017 02:16
@jjatie
Copy link
Collaborator

jjatie commented Jan 6, 2018

This seems to be a good change, but as @liuxuan30 said: What is the problem in the current library?

@LavareX
Copy link
Contributor Author

LavareX commented Jan 8, 2018

@liuxuan30 @jjatie Has no effect on the current library, repair logic is not rigorous, ignored the horizontal center.

@liuxuan30
Copy link
Member

again, we understand this should not impact the current library, we just need to know what's the change is about. You changed the x position and width to respect inset, we want to know the user case. Did you meet any issue? Or the x direction inset broke something on your side?

I guess when it was implemented, @danielgindi might considered this already, and intended only y direction.

@jjatie
Copy link
Collaborator

jjatie commented Jan 8, 2018

@yangcaimu This looks like a good change, we just want to document it better so if there is a question or issue in the future, we can quickly understand the justification behind what's there.

@LavareX
Copy link
Contributor Author

LavareX commented Jan 9, 2018

@liuxuan30 @jjatie

Thank you for your patience reply, I'll make it clearer.

At present, the text is centered because the alignment attribute of the _paragraphStyle attribute is assigned to .center. When I change to .left, the text will be completely on the left side of the border, even modify the insets.

If you do not understand, I can provide demo.

@liuxuan30
Copy link
Member

liuxuan30 commented Jan 10, 2018

ok. So you change to .left, the text aligned on the left is desired result. When you add the inset, what you want to achieve? Adjust the frame x by changing inset?

@LavareX
Copy link
Contributor Author

LavareX commented Jan 11, 2018

Yes, especially in the case of multiple lines, to achieve the effect of left and right edge by changing inset.

wx20180111-103250 2x

@liuxuan30
Copy link
Member

@yangcaimu can you post compare images before/after changing the inset?

@thierryH91200
Copy link
Contributor

there is also this solution

RectMarker.swift.zip

capture d ecran 2018-01-11 09 52 57
capture d ecran 2018-01-11 09 53 05

@LavareX
Copy link
Contributor Author

LavareX commented Jan 11, 2018

@liuxuan30

Before:
inset = UIEdgeInsets.zero

wx20180111-103250 2x

After:
insets = UIEdgeInsets(top: 8, left: 8, bottom: 20, right: 8)

wx20180111-180248 2x

@jjatie
Copy link
Collaborator

jjatie commented Jan 11, 2018

@liuxuan30 I like this change.

While it's already currently possible to achieve the same look, I think this way is much more intuitive.

If you're okay with it let's merge.

@jjatie jjatie added this to the 3.1 milestone Jan 12, 2018
@liuxuan30
Copy link
Member

sure thing. once we know how it looks after fix, it makes sense then.

@liuxuan30 liuxuan30 merged commit ba40ab4 into ChartsOrg:master Jan 15, 2018
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.

5 participants