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

Add borderDash options to arc element #11127

Merged
merged 1 commit into from
Feb 12, 2023

Conversation

stockiNail
Copy link
Contributor

Fix #11126

@etimberg etimberg added this to the Version 4.3.0 milestone Feb 9, 2023
const inner = options.borderAlign === 'inner';

if (!borderWidth) {
return;
}

ctx.setLineDash(borderDash || []);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You set the defat to an empty array so this check is not necessary then right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that was my doubt as well. But I have seen in the other parts of code, everytime this check is done for borderDash because setLineDash fails if not an array, I guess. Therefore not clear to me if it's not correct on the other parts or if better to leave this check anyway :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LeeLenaleee don't ask me why because I don't have the answer now (I checked this morning without success) but it seems that the donut options doesn't fallback to element ones therefore if you set borderDash: null in dataset config (and you don't have those check), you have an issue. This weird behavior was already recognized I guess because the _indexable about spacing option was added to the controller instead of the element. If you agree, I can pend more time to go in deep why the donut doesn't fallback to the arc element (maybe in another PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LeeLenaleee I have checked more in deep and the descriptors are working well. I wasn't aware that the resolver uses the descriptors of the default of the controller (and not in cascade as I was thinking) therefore it's working as designed.

About the point of your review, I can confirm that if you set borderDash: null, you have an error (but fallback works correctly if you set to undefined).
The check where null is passed as defined is here:

if (defined(value)) {

The define function checks if the value is undefined but with null argument returns true. Probably the define function could be remove, using instead isNullOrUndef. WDYT?

In my opinion, but I can be wrong, it would be better to maintain that check anyway in the meanwhile. But feel free to disagree ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Null is considered a valid value for configuration while undefined is considered not defined and thus fallback. Falling back from null would be a breaking change, extending to all unknown use cases.

Copy link
Contributor Author

@stockiNail stockiNail Feb 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kurkle yes!

EDIT: this is why I would suggest to stay as is, in this PR .

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of agree with @LeeLenaleee, but the behaviour should be changed everywhere to be consistent, so maybe do at v5 as a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kurkle @LeeLenaleee @etimberg I think we agreed to change for v5. Nevertheless let me outline that there are other options/situation which should be addressed, similar to this one, I guess. Maybe you are already aware but let me make an example.

The doughnut controller is setting spacing options in the default, set to 0. doing that, the spacing option of the element is ignored, therefore having a following chart config

{
  type: 'doughnut',
  data: {
    labels: [0, 1, 2, 3, 4, 5],
    datasets: [
      {
        data: [0, 2, 4, null, 6, 8],
        cutout: 200
      }]
  },
  options: {
    elements: {
      arc: {
        spacing: 30
      }
    }
  }
}

the spacing: 30 is ignored in the cutout/maxsize calculation because it uses 0 (controller defaults) instead of 30. This is happening because the dataset options are not using the fallback in the update phase of the controller and when you have to use options of data element during the update of the controller, you must check against a default or replicate the options definition, ignoring the element config value/defaults. @kurkle I think we have already talked about that in the treemap controller.

@@ -76,7 +76,7 @@ export default class DoughnutController extends DatasetController {

static descriptors = {
_scriptable: (name) => name !== 'spacing',
_indexable: (name) => name !== 'spacing',
_indexable: (name) => name !== 'spacing' && !name.startsWith('borderDash') && !name.startsWith('hoverBorderDash'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why startsWith? and probably does not need hoverBorderDash, as its returned as borderDash when hovered.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kurkle I used startsWith to manage also borderDashOffset at the say way of borderDah because the 2 options are strictly related.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably does not need hoverBorderDash, as its returned as borderDash when hovered.

It seems you're right. I was convinced that the indexable policy is ignored for hover if not specified but it doesn't seem so. I'll submit another PR to remove it.

const inner = options.borderAlign === 'inner';

if (!borderWidth) {
return;
}

ctx.setLineDash(borderDash || []);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of agree with @LeeLenaleee, but the behaviour should be changed everywhere to be consistent, so maybe do at v5 as a breaking change.

@etimberg etimberg merged commit 7f9bca6 into chartjs:master Feb 12, 2023
@stockiNail stockiNail deleted the borderDashArcElement branch February 13, 2023 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing borderDash option to arc element
4 participants