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.

Sign up for GitHub

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

Jump to bottom

Add stroke lines option around ticks to improve readability #6787

Merged
merged 8 commits into from
Jan 12, 2020
Merged

Add stroke lines option around ticks to improve readability #6787

merged 8 commits into from
Jan 12, 2020

Conversation

joseraul
Copy link
Contributor

@joseraul joseraul commented Nov 24, 2019 •
edited
Loading

I come from here: #6784

Before: https://codepen.io/joseraul/pen/qBBwgpQ

After:

ticks: {
    ...
    strokeStyle: 'white',
    lineWidth: 3,
    ...
}

image

@joseraul joseraul changed the title dd stroke lines option around ticks to improve readability Add stroke lines option around ticks to improve readability Nov 24, 2019
@joseraul joseraul mentioned this pull request Nov 24, 2019
Closed
etimberg
etimberg reviewed Nov 24, 2019
View reviewed changes
src/core/core.scale.js Outdated Show resolved Hide resolved
src/core/core.defaults.js Outdated Show resolved Hide resolved
@joseraul joseraul requested a review from etimberg November 24, 2019 20:30
@joseraul joseraul requested a review from benmccann November 27, 2019 16:31
benmccann
benmccann reviewed Nov 27, 2019
View reviewed changes
src/core/core.scale.js Outdated
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

kurkle
kurkle reviewed Nov 27, 2019
View reviewed changes
src/core/core.scale.js Outdated
@@ -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 •
edited
Loading

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.

@joseraul joseraul requested review from kurkle and benmccann December 1, 2019 17:31
@joseraul
Copy link
Contributor Author

joseraul commented Dec 1, 2019

Sorry for the delay! I think is ready now, could you take a look when you have some time, please? thanks!

BTW, @benmccann, @etimberg and @kurkle, thanks for supporting the project... 491 issues and only 11 PR's... it's a pity that there are no more developers that try to collaborate a bit, so, thanks for your work guys.

benmccann
benmccann previously approved these changes Dec 1, 2019
View reviewed changes
Copy link
Contributor

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

this looks okay to me. @etimberg and @kurkle will typically take a look as well before a PR is merged

kurkle
kurkle reviewed Dec 10, 2019
View reviewed changes
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.

Looks like there is still some minor adjustment to do to get this fully functional. I could be wrong on those points though :)

src/core/core.scale.js Outdated
@@ -1255,16 +1259,20 @@ class Scale extends Element {
ctx.fillStyle = tickFont.color;
ctx.textBaseline = 'middle';
ctx.textAlign = item.textAlign;
ctx.strokeStyle = optionTicks.strokeStyle;
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!

src/core/core.scale.js Outdated
@@ -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

@kurkle kurkle Dec 10, 2019 •
edited
Loading

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.

@joseraul
Copy link
Contributor Author

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

You are right 😞 , major is ignoring the stroke... @kurkle any idea? Looks a little confusing to me how the text is being generated...

@joseraul joseraul requested a review from kurkle January 10, 2020 17:36
kurkle
kurkle reviewed Jan 10, 2020
View reviewed changes
src/core/core.scale.js Outdated Show resolved Hide resolved
kurkle
kurkle reviewed Jan 10, 2020
View reviewed changes
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.

Can you debug again, if those options are now correctly resolved into the font?

kurkle
kurkle reviewed Jan 10, 2020
View reviewed changes
src/core/core.scale.js Outdated Show resolved Hide resolved
@joseraul joseraul requested a review from kurkle January 10, 2020 19:17
kurkle
kurkle previously approved these changes Jan 10, 2020
View reviewed changes
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.

Lgtm

etimberg
etimberg previously approved these changes Jan 10, 2020
View reviewed changes
@etimberg
Copy link
Member

@benmccann did you want to review this as well?

benmccann
benmccann reviewed Jan 11, 2020
View reviewed changes
src/core/core.scale.js Outdated Show resolved Hide resolved
benmccann
benmccann reviewed Jan 11, 2020
View reviewed changes
src/core/core.scale.js Outdated Show resolved Hide resolved
benmccann
benmccann reviewed Jan 11, 2020
View reviewed changes
src/core/core.scale.js Outdated Show resolved Hide resolved
@benmccann
Copy link
Contributor

This looks good to me. I just had a couple nitpicks

@joseraul joseraul dismissed stale reviews from etimberg and kurkle via 028102b January 12, 2020 08:41
@joseraul joseraul requested a review from benmccann January 12, 2020 08:43
@etimberg etimberg added this to the Version 3.0 milestone Jan 12, 2020
@etimberg etimberg merged commit b1421c6 into chartjs:master Jan 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kurkle kurkle kurkle approved these changes

@etimberg etimberg etimberg approved these changes

@benmccann benmccann benmccann approved these changes

Assignees
No one assigned
Labels
type: enhancement
Projects
None yet
Milestone
Version 3.0
Development

Successfully merging this pull request may close these issues.
4 participants
@joseraul @etimberg @benmccann @kurkle