chartjs / chartjs-plugin-annotation

Annotation plugin for Chart.js
MIT License
603 stars 325 forks source link

Change borderCapStyle and borderJoinStyle defaults to be compatible with SKIA canvas #939

Closed mlohbihler closed 5 days ago

mlohbihler commented 1 month ago

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

LeeLenaleee commented 1 month ago

Change LGTM, but it seems the CI fails

mlohbihler commented 1 month ago

Change LGTM, but it seems the CI fails

I'll have a look.

mlohbihler commented 1 month ago

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 commented 1 month ago

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?

LeeLenaleee commented 1 month ago

I will look into it this weekend

stockiNail commented 1 month ago

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

stockiNail commented 1 week ago

@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

mlohbihler commented 1 week ago

@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.

stockiNail commented 1 week ago

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

mlohbihler commented 1 week ago

@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 commented 5 days ago

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

stockiNail commented 5 days ago

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.