-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
fixes pie label conflict resolution #15410
Conversation
@@ -244,7 +244,7 @@ export function VislibVisualizationsPieChartProvider(Private) { | |||
x: pos[0], | |||
y: pos[1], | |||
left: midAngle < Math.PI ? pos[0] : pos[0] - bbox.width, | |||
right: midAngle > Math.PI ? pos[0] + bbox.width : pos[0], | |||
right: midAngle < Math.PI ? pos[0] + bbox.width : pos[0], |
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.
why this flip?
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.
ups, that shouldn't be there. thanks!
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.
LGTM
* fixes pie label conflict resolution * fixing based on review
* fixes pie label conflict resolution * fixing based on review
@@ -258,8 +258,8 @@ export function VislibVisualizationsPieChartProvider(Private) { | |||
const current = d.position; | |||
if (point) { | |||
const horizontalConflict = (point.left < 0 && current.left < 0) || (point.left > 0 && current.left > 0); | |||
const verticalConflict = (point.top > current.top && point.top <= current.bottom) || | |||
(point.top < current.top && point.bottom >= current.top); | |||
const verticalConflict = (point.top >= current.top && point.top <= current.bottom) || |
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.
This fix does solve the conflict but It hide the label which conflict with other which looks like a bug. (missing label)
What it should do is to try to find a better place to be shown. Usually there is still plenty of space
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.
The same happens with labels on the same level as well, if conflict is detected its just hidden. Could you open a separate enhancement request for trying to find new position for the label if it is in conflict with another ? (i had this implemented but we decided to move it out of the original PR as the algorithm i was using was resource intensive)
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.
where can I see the code for trying to find new position for the label if it is in conflict with another?
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.
In addition ,I opened feature request: #15571
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.
resolves #15406
scenario when two labels would be positioned at exactly the same place was not handled correctly