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

Enhance legend label color point when usePointStyle is true #6006

Merged
merged 10 commits into from
Feb 10, 2019
Merged

Enhance legend label color point when usePointStyle is true #6006

merged 10 commits into from
Feb 10, 2019

Conversation

alfiehopkin
Copy link
Contributor

Allow for users to add in an extra parameter to the label options which changes the size of the legend box separate to the font size. This doesn't affect current functionality, but adds to it.

Allow for users to add in an extra parameter to the label options which changes the size of the legend box separate to the font size. This doesn't affect current functionality, but adds to it.
@benmccann
Copy link
Contributor

This would have to be added to the documentation. I don't quite understand the feature, but haven't spent much time looking at the PR yet. Can you share a jsfiddle, codepen, or screenshot of what this looks like before and after the change to help demonstrate what you're changing?

@alfiehopkin
Copy link
Contributor Author

alfiehopkin commented Jan 23, 2019

Sorry for little information in the previous, and sorry for the delay in reply.

I understand (and agree) that this would have to be added to the documentation.

The reason for this PR is that I often find myself having to build custom legends for charts, even though all I really want to do it have the standard legend, with circles instead of rectangles. This is possible but then you have to deal with the circle being the same size as the font size, where I want to have the ability to customise the size of the circle independently to the font size.

The proposed solution does not break any current usage, instead it just adds the ability to override the default of using the font size with a custom value.

What currently happens:
screenshot 2019-01-23 at 20 20 41

What I have proposed:
screenshot 2019-02-10 at 15 48 38

The legend options code would then be as follows (instead of having to create a custom legend, and events):
screenshot 2019-01-23 at 20 23 27
Removing the circleSize would then default to using the fontSize because we have set usePointStyle: true.

Hope that this helps describe the PR in more detail.

EDIT:
Also circleSize could be changed to pointSize to be more generic.

Fixed the position of the legend box, when using custom point sizes.
@benmccann
Copy link
Contributor

+1 to calling it pointSize instead of circleSize

@alfiehopkin
Copy link
Contributor Author

@benmccann I have changed circleSize to pointSize and added pointSize to the documentation in my recent commits.

Should this now go to review?

@benmccann
Copy link
Contributor

I don't know this section of the code all that well, so I'll let the others review it. My main question is why do we need boxWidth and pointSize? Why not just use boxWidth in both cases? Otherwise it looks good to me

@alfiehopkin
Copy link
Contributor Author

@benmccann To be honest, I'm not sure why the functionality isn't allowed. There is a function called getBoxWidth which is in place to prevent the use of boxWidth and replace it with fontSize if usePointStyle is true. I don't know the reasoning for this so I just found it best to build on top an override instead of messing with the underlying code.

Can you label this as waiting for review? I can't do this for some reason.

@kurkle
Copy link
Member

kurkle commented Jan 25, 2019

This getBoxWidth was introduced in #3323 (to fix #3292). But the math was introduced already in the original PR #2902 that introduced usePointStyle.

I think the reasoning behind is to provide good default, when usePointStyle: true (40 would be a bit large in most cases)

IMO this is a configuration resulution issue and boxWidth should be obeyed if user supplied it, regardless of usePointStyle. And secondly the provided default seems a bit large?
The problem is that those defaults get merged to options and we really don't know if it was supplied by user.

So I would not add a new option for this. Instead I would fix the math for better default and honor user provided value over default in any case (this second part might be another PR).
What do others think?

@alfiehopkin
Copy link
Contributor Author

@kurkle I agree that this could just involve fixing the existing code, I just didn't want to break any existing functionality with this change.

I will look into this in more detail and remove the additional option, and instead work on a fix to allow for the user to override the default with the current boxWidth option.

@simonbrunel
Copy link
Member

simonbrunel commented Jan 25, 2019

I agree we shouldn't introduce a new option, though I wouldn't change the default (40), neither try to change the resolution order. To me, the point size should be min(boxWidth, boxHeight) (i.e. the circle inscribed in the label box), so basically min(boxWidth, fontSize).

@alfiehopkin
Copy link
Contributor Author

@simonbrunel sounds like a better approach again that.

@kurkle
Copy link
Member

kurkle commented Jan 25, 2019

@simonbrunel I was talking about the default when usePointStyle: true (= the math). I think the font.lineHeight could be even better than font.size (and radius of just half of that?).

Also I don't see why its calculated inside those loops in fit, nothing changes per item.

@simonbrunel
Copy link
Member

simonbrunel commented Jan 25, 2019

I'm not sure about font.lineHeight because the line height includes the space between each lines. Here I think we want the point to be the same size as the text, so to me it should be something like:

if (opts.labels && opts.labels.usePointStyle) {
    var radius = Math.min(boxWidth, fontSize) / 2;
    // ...

Alfie Hopkin added 2 commits January 25, 2019 11:46
Takes min value from boxSize and fontSize when using point styles for legend boxes, and correctly displays them on the screen.
@alfiehopkin
Copy link
Contributor Author

@simonbrunel @kurkle in the recent commits, default functionality remains the same when usePointStyle is set to false. When true, the box width is based on min(boxWidth, fontSize). The positioning is accurate and inline on the y axis as should be also.

I have also updated the docs to suit changes.

@kurkle
Copy link
Member

kurkle commented Jan 25, 2019

A pen to play with (b6acb9f): https://codepen.io/kurkle/full/pGgJMZ

And previous (80f117d): https://codepen.io/kurkle/full/GzopJm

@alfiehopkin
Copy link
Contributor Author

What is the next step for this PR?

@kurkle
Copy link
Member

kurkle commented Jan 27, 2019

What is the next step for this PR?

Wait for reviews

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.

plugins.legend.js is not quite up to the coding standards. So I'll ignore the surroundings on focus on change.

src/plugins/plugin.legend.js Outdated Show resolved Hide resolved
@benmccann
Copy link
Contributor

benmccann commented Jan 29, 2019

The current proposed solution seems a bit non-intuitive to me. It seems strange to me that changing the shape should change the size as well. Doing the minimum of the two values also seems weird because it means that boxWidth can only make the point smaller, but you can never make it bigger.

One problem is that boxWidth was really only meant to control the width and now we're trying to use it for the height. Maybe we also need a boxHeight option? The boxHeight could be undefined by default and fall back to fontSize in that case. If set it could control the height regardless of the value of usePointStyle

@benmccann
Copy link
Contributor

@alfiehd you'll need to rebase this PR

@alfiehopkin alfiehopkin dismissed stale reviews from etimberg and kurkle via ee54e97 February 4, 2019 13:25
@alfiehopkin
Copy link
Contributor Author

@kurkle @etimberg I had to merge changes with conflicts in the legend.md doc file.

@benmccann I am not familiar with rebasing. I've done some research but is there a specific procedure in this repo?

@benmccann
Copy link
Contributor

@alfiehd it looks like you did the rebase correctly. By rebase I mean merge the latest changes from master to remove any merge conflicts so that we can merge this PR. Thanks!

@alfiehopkin
Copy link
Contributor Author

@benmccann ahh right! All done, thanks!

@benmccann
Copy link
Contributor

The point looks too high to me in the screenshot in #6006 (comment). Is it possible to vertically center the point?

@simonbrunel simonbrunel added this to the Version 2.8 milestone Feb 9, 2019
@simonbrunel simonbrunel changed the title Allow custom pointStyle size Enhance legend label color point when usePointStyle is true Feb 9, 2019
@alfiehopkin
Copy link
Contributor Author

@benmccann I’ve already fixed the vertical positioning in the commit 2989223 - Fixed positioning of legend box

@benmccann
Copy link
Contributor

Ok. Would it be possible to update the screenshot?

@alfiehopkin
Copy link
Contributor Author

@benmccann updated :)

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.

Thanks for your patience on this one

@simonbrunel simonbrunel merged commit af464f8 into chartjs:master Feb 10, 2019
@simonbrunel
Copy link
Member

Thanks @alfiehd

@alfiehopkin
Copy link
Contributor Author

@benmccann @simonbrunel @kurkle thanks for all of your input on this one. Really glad it's been merged haha. My first PR ever, and with a library that I use on a daily basis and have created many plugins for internal use. I'll get around to sharing those with the community soon also!

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.

6 participants