chartjs-plugin-annotation icon indicating copy to clipboard operation
chartjs-plugin-annotation copied to clipboard

Fix defaults for skia canvas

Open mlohbihler opened this issue 1 year ago • 5 comments

Setting these defaults prevents errors when using annotations in the skia canvas context. See Issue for more details: https://github.com/chartjs/chartjs-plugin-annotation/issues/938

mlohbihler avatar Aug 16 '24 15:08 mlohbihler

Change LGTM, but it seems the CI fails

LeeLenaleee avatar Aug 22 '24 20:08 LeeLenaleee

Change LGTM, but it seems the CI fails

I'll have a look.

mlohbihler avatar Aug 22 '24 21:08 mlohbihler

I think i need help with this. test-integration works locally for me, but i'm on mac. Not sure why it would be failing on ubuntu.

Edit: Actually, it looks like test-integration succeeded, but then the process failed with no further messages.

mlohbihler avatar Aug 23 '24 15:08 mlohbihler

I think i need help with this. test-integration works locally for me, but i'm on mac. Not sure why it would be failing on ubuntu.

Edit: Actually, it looks like test-integration succeeded, but then the process failed with no further messages.

Actually, i'm getting the same build failure on the master branch. @LeeLenaleee can you check?

mlohbihler avatar Aug 23 '24 15:08 mlohbihler

I will look into it this weekend

LeeLenaleee avatar Aug 23 '24 15:08 LeeLenaleee

@mlohbihler LGTM. May be you can try the branch against SKIA canvas to check if it is working as expected.

stockiNail avatar Aug 26 '24 06:08 stockiNail

@mlohbihler the failed test has been fixed in https://github.com/chartjs/chartjs-plugin-annotation/pull/902. Therefore after merging of that and rebase this branch, we could re-run the failed job

stockiNail avatar Sep 18 '24 11:09 stockiNail

@mlohbihler the failed test has been fixed in #902. Therefore after merging of that and rebase this branch, we could re-run the failed job

@stockiNail nice. Thanks for this.

mlohbihler avatar Sep 18 '24 15:09 mlohbihler

@mlohbihler if you will rebase and push again, we can go on. Take your time. Thanks a lot!

stockiNail avatar Sep 19 '24 07:09 stockiNail

@mlohbihler if you will rebase and push again, we can go on. Take your time. Thanks a lot!

Build checks are passing now, but i guess with the (net zero) playing around i did i lost my approval. Re-requested.

mlohbihler avatar Sep 19 '24 19:09 mlohbihler

Apologize if I hadn't time before to have a look.

The changes are adding options not used by those annotations therefore I would prefer to change only the setBorderStyle (reverting the current changes of PR) as following:

export function setBorderStyle(ctx, options) {
  if (options && options.borderWidth) {
    ctx.lineCap = options.borderCapStyle || 'butt';
    ctx.setLineDash(options.borderDash);
    ctx.lineDashOffset = options.borderDashOffset;
    ctx.lineJoin = options.borderJoinStyle  || 'miter';
    ctx.lineWidth = options.borderWidth;
    ctx.strokeStyle = options.borderColor;
    return true;
  }
}

Let me know what you think

That change seems to work for me. Note that there are other instances of these properties getting set. Not sure if they can be removed too. E.g. see https://github.com/mlohbihler/chartjs-plugin-annotation/blob/0bce23d13a8db9aa9c58a80e5fc2929b4dfbab71/src/types/box.js#L40

mlohbihler avatar Sep 24 '24 14:09 mlohbihler

That change seems to work for me. Note that there are other instances of these properties getting set. Not sure if they can be removed too. E.g. see https://github.com/mlohbihler/chartjs-plugin-annotation/blob/0bce23d13a8db9aa9c58a80e5fc2929b4dfbab71/src/types/box.js#L40

@mlohbihler the joinStyle and capstyle are used for setting on canvas only in the point that you have changed. The other istances are only in the options defintions of the annotations in order to enable the options settings from the user.

stockiNail avatar Sep 25 '24 11:09 stockiNail