Enable label `position` as percentage of the size

[stockiNail]

Fix #553

TODO

• [x] line annotation
• [x] box annotation
• [x] label annotation
• [x] test cases
• [x] types
• [x] documentation

[stockiNail]

I have tested your suggestions but it seems something is not correct:

It seems the positions are not correct

[stockiNail]

Ok, now I understand better your commits and there is a misunderstanding.

The position as number is not in PIXEL but is a value between 0 and 1 (as percentage) but numeric. I don't know if makes sense to allow the number as position in PX because there is already x/yAdjust options for that.

[stockiNail]

If we don't to provide the number as percentage, it's better to remove it and enable only the percentage as string. What do you think?

[kurkle]

Ok, now I understand better your commits and there is a misunderstanding.

The position as number is not in PIXEL but is a value between 0 and 1 (as percentage) but numeric. I don't know if makes sense to allow the number as position in PX because there is already x/yAdjust options for that.

If we don't to provide the number as percentage, it's better to remove it and enable only the percentage as string. What do you think?

I don't think having 2 ways to do the same thing (define percentage) is good. It creates many code paths for the same result and could be confusing to users.

So, I agree, it would be better to remove and only enable the percentage string.

I think a good set of helpers could be (would also make the width: 'invalid string' option for images fall back to the actual width):

const isPercentString = (s) => typeof s === 'string' && s.endsWith('%');
const toPercent = (s) => parseFloat(s) / 100;

export function getRelativePosition(size, positionOption) {
  if (positionOption === 'start') {
    return 0;
  }
  if (positionOption === 'end') {
    return size;
  }
  if (isPercentString(positionOption)) {
    return toPercent(positionOption) * size;
  }
  return size / 2;
}

export function getSize(size, value) {
  if (typeof value === 'number') {
    return value;
  } else if (isPercentString(value)) {
    return toPercent(value) * size;
  }
  return size;
}

Again my thoughts only, I can agree to other implementations too.

[stockiNail]

OK, fine for me! Let me change the code and test it!

[stockiNail]

the getRelativePosition is working well only with position 'start'. I need to refactor it.

[kurkle]

the getRelativePosition is working well only with position 'start'. I need to refactor it.

It's assuming you remove paddings and margins etc extra adjustments and pass the actual available size.

But I'm not 100% on the case, so I might be very wrong :)

[stockiNail]

It's assuming you remove paddings and margins etc extra adjustments and pass the actual available size.

yes, and forgive my lack of knowledge. But for instance, setting 'end', this is returning the size but I can not add to start point but I need to calculate the space anyway. And to do that, I need to re-check the position value (if statement). The same for center where I have to take care about the label size.

A label is drawn passing the box where the label must be drawn (see also textAlign).

For what I can see now (but open to change my idea) the best is to have something like was implemented before....

But again, feel free to correct me because I can not see another good way to do it.

[stockiNail]

In my poor opinion, trying to "centralize" the logic of the position, we could risk to increase complexity.

[kurkle]

See https://github.com/pepstock-org/chartjs-plugin-annotation/pull/1

[stockiNail]

See pepstock-org#1

@kurkle really thank you for support! I really appreciate.

They are code climate similar code... But as we did before, we could accept it.

[kurkle]

Yes, we can ignore the codeclimate here.

But there are still things to do.. The docs need to be updated, as it was agreed not to accept the number variant of the percantege. Also the getTPoistion function is unused I think.

[stockiNail]

yes, I am also rebasing to have the another PR.. need some minutes (let me finish another stuff, unfortunately not related to this).

[stockiNail]

Yes, we can ignore the codeclimate here.

How can I set it to be ignored? :(

[kurkle]

Yes, we can ignore the codeclimate here.

How can I set it to be ignored? :(

Don't look at it :)

[kurkle]

Only one mistake left, made by me. I'll commit the fix directly.

Hmm, I can't. Maybe the "allow cnages by maintainers" flag was not checked in the PR.

Also it needs a update to the fixture, so can't do that directly here too :)

[stockiNail]

Only one mistake left, made by me. I'll commit the fix directly.

Hmm, I can't. Maybe the "allow cnages by maintainers" flag was not checked in the PR.

Also it needs a update to the fixture, so can't do that directly here too :)

I can do it.