Skip to content

Conversation

@asmenezes
Copy link
Contributor

Added Stroke Color and Width to Cartesian scale options, and pass them to drawTitle() so they can be used on the titles for Cartesian scales

Example: https://codepen.io/asmenezes/pen/zxrzaEx

Copy link
Collaborator

@LeeLenaleee LeeLenaleee left a comment

Choose a reason for hiding this comment

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

Can you also add a test for this?

@asmenezes
Copy link
Contributor Author

Can you also add a test for this?

Added an image based test. Let me know if that's not the appropriate way to do it.

@etimberg
Copy link
Member

etimberg commented Oct 14, 2025

@asmenezes TBH, not sure if an image based test works here. We have a way of spriting text during tests to handle browser inconsistencies so I suspect it doesn't actually exercise the stroke code

@asmenezes
Copy link
Contributor Author

@etimberg Thanks for the info. I have been thinking about this and I am not sure if I can think of any other way to test that the stroke has actually been drawn.

I did consider a test similar to the many that resolve like the example below.

  var scale = chart.scales.x;
  expect(getLabels(scale)).toEqual(labels.slice(1));

However I don't know if that tests if the stroke color and width were actually used, or if they were just added as properties.

Do you know if that sort of test is sufficient or I should find another way to do it?

Thanks.

@etimberg
Copy link
Member

@asmenezes I think that test would be sufficient here

@asmenezes
Copy link
Contributor Author

@etimberg Thanks.

etimberg
etimberg previously approved these changes Oct 19, 2025
@asmenezes asmenezes dismissed etimberg’s stale review October 19, 2025 14:30

The merge-base changed after approval.

LeeLenaleee
LeeLenaleee previously approved these changes Oct 20, 2025
@asmenezes asmenezes dismissed LeeLenaleee’s stale review October 20, 2025 13:44

The merge-base changed after approval.

etimberg
etimberg previously approved these changes Oct 20, 2025
@asmenezes asmenezes dismissed etimberg’s stale review October 20, 2025 13:51

The merge-base changed after approval.

LeeLenaleee
LeeLenaleee previously approved these changes Oct 20, 2025
@asmenezes asmenezes dismissed LeeLenaleee’s stale review October 20, 2025 15:37

The merge-base changed after approval.

@LeeLenaleee
Copy link
Collaborator

@asmenezes it seems like something is not working right, it automatically dismisses reviews.
Can you try pulling in the latest master, since some of your changes are the same as currently in master to see if that fixes the issue.

@asmenezes
Copy link
Contributor Author

@LeeLenaleee should be all synced up now

@etimberg etimberg merged commit e8c58da into chartjs:master Oct 20, 2025
7 checks passed
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.

3 participants