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

Enable applying of gradients and pattern on line segments #11217

Merged
merged 3 commits into from Apr 27, 2023

Conversation

stockiNail
Copy link
Contributor

Fix #11204

Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

I think it should still return false if prevStyle is undefined. Also might be worth comparing the object instances for equality when its pattern or gradient

@stockiNail
Copy link
Contributor Author

I think it should still return false if prevStyle is undefined.

You're right! I'll do

Also might be worth comparing the object instances for equality when its pattern or gradient

Ok, therefore scans the object properties checking if the values are equals. right?

@kurkle
Copy link
Member

kurkle commented Mar 30, 2023

Thats what I was thinking. Maybe its ok to just check each property and not use JSON.stringify at all..

@stockiNail
Copy link
Contributor Author

Maybe its ok to just check each property and not use JSON.stringify at all..

Fully agree. I'm busy now in a couple of other things. As soon as I'll finish, I'll change it and re-commit. I was checking before if there is already a helpers function which is doing that but I don't think so.

@stockiNail stockiNail marked this pull request as draft March 30, 2023 16:33
@stockiNail
Copy link
Contributor Author

Maybe its ok to just check each property and not use JSON.stringify at all..

it sounds more complex than I expected... Let me propose another options using stringify.

@stockiNail stockiNail requested a review from kurkle March 30, 2023 19:09
@stockiNail stockiNail marked this pull request as ready for review March 30, 2023 19:09
if (!cache.includes(value)) {
cache.push(value);
}
return cache.indexOf(value);
Copy link
Member

Choose a reason for hiding this comment

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

A map could be more efficient in these. But I'm not really worried about that. My main worry is a confusion between index in the array and some other possible numeric value, which is probably a non-issue really.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking to use a map but the array could contain max 4 objects.
Not clear about the confusion. The index is used only to compare a gradient/pattern. We needed a unique value per object instance to put in the json. It seemed to me better than a string or object.

@etimberg etimberg added this to the Version 4.3.0 milestone Mar 30, 2023
@etimberg etimberg merged commit ee7e928 into chartjs:master Apr 27, 2023
5 checks passed
@stockiNail stockiNail deleted the fixSegment branch April 28, 2023 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to set different line colors based on marker values and color in chart js line graph
3 participants