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 stroke lines option around ticks to improve readability #6787

Merged
merged 8 commits into from
Jan 12, 2020
Merged
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/axes/styling.md
Original file line number Diff line number Diff line change
@@ -43,9 +43,11 @@ The tick configuration is nested under the scale configuration in the `ticks` ke
| `fontStyle` | `string` | `'normal'` | Font style for the tick labels, follows CSS font-style options (i.e. normal, italic, oblique, initial, inherit).
| `lineHeight` | <code>number&#124;string</code> | `1.2` | Height of an individual line of text (see [MDN](https://developer.mozilla.org/en-US/docs/Web/CSS/line-height)).
| `reverse` | `boolean` | `false` | Reverses order of tick labels.
| `lineWidth` | `number` | `0` | Stroke width around the text.
| `minor` | `object` | `{}` | Minor ticks configuration. Omitted options are inherited from options above.
| `major` | `object` | `{}` | Major ticks configuration. Omitted options are inherited from options above.
| `padding` | `number` | `0` | Sets the offset of the tick labels from the axis
| `strokeStyle` | `string` | `` | The color of the stroke around the text.
| `z` | `number` | `0` | z-index of tick layer. Useful when ticks are drawn on chart area. Values &lt;= 0 are drawn under datasets, &gt; 0 on top.

## Minor Tick Configuration
17 changes: 15 additions & 2 deletions src/core/core.scale.js
Original file line number Diff line number Diff line change
@@ -53,6 +53,8 @@ defaults._set('scale', {
minRotation: 0,
maxRotation: 50,
mirror: false,
lineWidth: 0,
strokeStyle: '',
padding: 0,
display: true,
autoSkip: true,
@@ -199,7 +201,9 @@ function parseFontOptions(options, nestedOpts) {
fontFamily: valueOrDefault(nestedOpts.fontFamily, options.fontFamily),
fontSize: valueOrDefault(nestedOpts.fontSize, options.fontSize),
fontStyle: valueOrDefault(nestedOpts.fontStyle, options.fontStyle),
lineHeight: valueOrDefault(nestedOpts.lineHeight, options.lineHeight)
lineHeight: valueOrDefault(nestedOpts.lineHeight, options.lineHeight),
benmccann marked this conversation as resolved.
Show resolved Hide resolved
lineWidth: valueOrDefault(nestedOpts.lineWidth, options.lineWidth),
kurkle marked this conversation as resolved.
Show resolved Hide resolved
strokeStyle: valueOrDefault(nestedOpts.strokeStyle, options.strokeStyle),
benmccann marked this conversation as resolved.
Show resolved Hide resolved
}), {
color: resolve([nestedOpts.fontColor, options.fontColor, defaults.global.defaultFontColor])
});
@@ -209,7 +213,12 @@ function parseTickFontOptions(options) {
var minor = parseFontOptions(options, options.minor);
var major = options.major.enabled ? parseFontOptions(options, options.major) : minor;

return {minor: minor, major: major};
return {
minor: minor,
major: major,
strokeStyle: valueOrDefault(options.strokeStyle, defaults.scale.ticks.strokeStyle),
Copy link
Contributor

Choose a reason for hiding this comment

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

These options should probably be included in parseFontOptions instead

We have a notion of major & minor ticks, which is best demonstrated here: https://www.chartjs.org/samples/latest/scales/time/financial.html. In that example we have the years as major ticks and have them formatted differently. Putting in parseFontOptions will allow the users to have different strokeStyle and lineWidth for major and minor ticks. While I don't have a specific use case in mind for that it would be more consistent with how the other options work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I think I'm missing something.
It is already in parseFontOptions, but if for example, I don't add it here or in _parseFont, the arguments are not set, I mean strokeStyle and lineWidth are empty... where is the "setter"? I don't find it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oki! I let the parseTickFontOptions how it was and the new options are only in parseFontOptions

lineWidth: valueOrDefault(options.lineWidth, defaults.scale.ticks.lineWidth)
};
}

function nonSkipped(ticksToFilter) {
@@ -1255,16 +1264,20 @@ class Scale extends Element {
ctx.fillStyle = tickFont.color;
ctx.textBaseline = 'middle';
ctx.textAlign = item.textAlign;
ctx.strokeStyle = optionTicks.strokeStyle;
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be tickFont.strokeStyle, I guess this is the missing link.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @kurkle, thanks for the suggestion, but unfortunately nop:

image

I need to explicitly add it to _parseFont to have it available in tickFont, but it seems that I'm adding this new options to everywhere that needs to parse fonts, so it's like a little dirty, doesn't it?

However, I can't find how to use the options directly inside _drawLabels, the new arguments are empty if I don't add in _parseFont or parseTickFontOptions .. so there is some magic somewhere that I can't find

Copy link
Member

Choose a reason for hiding this comment

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

You had those changes in parseFontOptions when debugging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, it's there lineWidth, strokeStyle:

function parseFontOptions(options, nestedOpts) {
	return helpers.extend(helpers.options._parseFont({
		fontFamily: valueOrDefault(nestedOpts.fontFamily, options.fontFamily),
		fontSize: valueOrDefault(nestedOpts.fontSize, options.fontSize),
		fontStyle: valueOrDefault(nestedOpts.fontStyle, options.fontStyle),
		lineHeight: valueOrDefault(nestedOpts.lineHeight, options.lineHeight),
		lineWidth: valueOrDefault(nestedOpts.lineWidth, options.lineWidth),
		strokeStyle: valueOrDefault(nestedOpts.strokeStyle, options.strokeStyle),
	}), {
		color: resolve([nestedOpts.fontColor, options.fontColor, defaults.global.defaultFontColor])
	});
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Finally, the options are in optionTicks and as I commented above, I let also the pareFontOptions for the minor and major.

Copy link
Member

Choose a reason for hiding this comment

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

I'd be ok with enabling it in _parseFont if we allow all the text to be stroked as well (likely outside the scope of this PR)

Copy link
Member

@kurkle kurkle Dec 10, 2019

Choose a reason for hiding this comment

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

I don't think specifying different lineWidth for major ticks works now.

ctx.lineWidth = optionTicks.lineWidth;
Copy link
Member

Choose a reason for hiding this comment

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

If my memory serves, ctx will not accept a lineWidth of 0 and remains in the previous value.
So I think we should add a check for lineWidth and not do the strokeText if its not defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good one, I did a small test and ctx does not accept zeros in lineWidth at all. I added a small conditional. thanks!


label = item.label;
y = item.textOffset;
if (isArray(label)) {
for (j = 0, jlen = label.length; j < jlen; ++j) {
// We just make sure the multiline element is a string here..
ctx.strokeText('' + label[j], 0, y);
ctx.fillText('' + label[j], 0, y);
y += tickFont.lineHeight;
}
} else {
ctx.strokeText(label, 0, y);
benmccann marked this conversation as resolved.
Show resolved Hide resolved
ctx.fillText(label, 0, y);
}
ctx.restore();
1 change: 1 addition & 0 deletions test/context.js
Original file line number Diff line number Diff line change
@@ -94,6 +94,7 @@ Context.prototype._initMethods = function() {
fill: function() {},
fillRect: function() {},
fillText: function() {},
strokeText: function() {},
lineTo: function() {},
measureText: function(text) {
// return the number of characters * fixed size
2 changes: 2 additions & 0 deletions test/specs/scale.category.tests.js
Original file line number Diff line number Diff line change
@@ -49,6 +49,8 @@ describe('Category scale tests', function() {
labelOffset: 0,
minor: {},
major: {},
lineWidth: 0,
kurkle marked this conversation as resolved.
Show resolved Hide resolved
strokeStyle: '',
}
});

2 changes: 2 additions & 0 deletions test/specs/scale.linear.tests.js
Original file line number Diff line number Diff line change
@@ -42,6 +42,8 @@ describe('Linear Scale', function() {
labelOffset: 0,
minor: {},
major: {},
lineWidth: 0,
strokeStyle: '',
}
});

2 changes: 2 additions & 0 deletions test/specs/scale.logarithmic.tests.js
Original file line number Diff line number Diff line change
@@ -42,6 +42,8 @@ describe('Logarithmic Scale tests', function() {
labelOffset: 0,
minor: {},
major: {},
lineWidth: 0,
strokeStyle: '',
},
});

2 changes: 2 additions & 0 deletions test/specs/scale.radialLinear.tests.js
Original file line number Diff line number Diff line change
@@ -63,6 +63,8 @@ describe('Test the radial linear scale', function() {
labelOffset: 0,
minor: {},
major: {},
lineWidth: 0,
strokeStyle: '',
},
});

2 changes: 2 additions & 0 deletions test/specs/scale.time.tests.js
Original file line number Diff line number Diff line change
@@ -95,6 +95,8 @@ describe('Time scale tests', function() {
major: {
enabled: false
},
lineWidth: 0,
strokeStyle: '',
},
time: {
parser: false,