Legend Symbol Parameter [New Feature] #4811

touletan · 2017-10-04T01:16:22Z

New Feature proposed to display symbol on legend
Legend Symbol Parameter [New Feature] #4804

benmccann · 2017-10-04T02:18:41Z

@touletan you can push updates to an existing PR. you didn't really have to close #4806. please make updates on the same PR in the future. let us know if you're not sure how to do that

etimberg · 2017-10-06T01:33:04Z

docs/configuration/legend.md

+ legendSymbolLarge: Boolean
+
+ // Width of legend symbol used if legendSymbolLarge is true
+ boxWidth: Number


My only comment here is that I think it would be better to remove legendSymbolLarge and imply it if boxWidth is set. So if boxWidth !== undefined then it will be used, otherwise it will be set to fontSize

if I'm not wrong, boxWidth is defaulted to 40 so it will be always defined. If we remove default value it may impact current users upgrading to that new version.

ahh, I missed that. To make that clearer I would suggest a rename to useFontSize or something. Let's wait to rename it until @simonbrunel gets a chance to review this

etimberg · 2017-10-06T01:33:52Z

src/helpers/helpers.canvas.js

@@ -44,112 +44,121 @@ var exports = module.exports = {
 }
 },

- drawPoint: function(ctx, style, radius, x, y) {
- var type, edgeLength, xOffset, yOffset, height, size;
+ drawPoint: function(ctx, style, width, height, x, y, isLineWidthZero) {


what is isLineWidthZero used for here?

For polygon (circle, rect, ....) if line width is 0, then no border are displayed (no strike).
This is replication of the current behavior for the rect symbol. Not doing this may impact the current users upgrading to that new version even if they don't change anything to their code (regression).

simonbrunel · 2017-10-06T12:02:59Z

The options logic path seems a bit unclear and I don't think the helpers.canvas.drawPoint should be changed (it's a public method and modifying its signature like you did is a breaking change). Though, I'm not sure to get the result of that feature very well, @touletan can you please post screenshots of the different use cases you are trying to implement?

touletan · 2017-10-06T13:31:22Z

@simonbrunel , my specific need is to use a large line as legend symbol while keeping the dot as point style on the lines.

I did few additional changes to be able to use other symbols that were already available as points.

All the logic about usePointStyle is to avoid breaking that current functionality.

Agree about the helper signature, it could break external plugin using it. I propose the rename the new method and keep the same logic in it. The code from the current method will be changed to call the new method from there and parameter values will be defaulted/updated so the outcome is the same.

simonbrunel · 2017-10-06T13:56:10Z

I think it's fine to add a new helpers.canvas.(draw)Symbol that split the radius into width and height and have drawPoint calling this method. Though, I don't like the extra isLineWidthZero parameter, which seems to only prevent ctx.stroke.

What about a new helpers.canvas.symbol method that simply creates the shape (no ctx.fill or ctx.stroke)? The calling code would be responsible to fill and/or stroke this shape:

drawPoint: (...) {
    helpers.canvas.symbol(ctx, style, w, h, x, y); // would be exports.symbol(...)
    ctx.stroke();
    ctx.fill();
}

The legend would use only the helpers.canvas.symbol method.

Then I would maybe expose the following dataset/legend options:

legend: {             // nested options under the dataset
    symbol: 'rect',   // default to rect
    boxWidth: ...
}

if usePointStyle is true, it would override symbol and boxWidth.

touletan · 2017-10-06T14:44:56Z

sounds like a plan. @etimberg, any other comments before I start working on it?

touletan · 2017-10-06T20:06:34Z

@simonbrunel, ctx.fill() is not required for each symbol, so it should stay in the method logic.

etimberg · 2017-10-07T00:42:46Z

@touletan I have no other comments. 😄

touletan · 2017-10-07T15:58:02Z

@etimberg , @simonbrunel , @benmccann , new version completed and ready for review.

…. Symbol type,, boxWidth, border Color and border Width can be defined at dataset level

etimberg · 2017-12-31T15:15:15Z

@simonbrunel have you had a chance to review this since it was last changed?

touletan · 2018-01-17T04:25:49Z

updates done

touletan · 2018-01-17T23:28:00Z

should be good now

benmccann · 2018-07-08T01:11:53Z

@touletan this PR needs to be rebased before it can be merged

benmccann · 2018-07-15T23:22:32Z

@touletan just a reminder that this PR needs to be rebased

touletan · 2018-07-17T03:18:26Z

Hi could you please provide guidelines to do this correctly. thank you.

…

________________________________ De : Ben McCann <notifications@github.com> Envoyé : 15 juillet 2018 23:23 À : chartjs/Chart.js Cc : touletan; Mention Objet : Re: [chartjs/Chart.js] Legend Symbol Parameter [New Feature] (#4811) @touletan<https://github.com/touletan> just a reminder that this PR needs to be rebased — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#4811 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AE_7QfxoNbPpFU_pwLBG_C0_qXaPrjK3ks5uG87VgaJpZM4Ps-yk>.

benmccann · 2018-07-17T04:52:10Z

I would recommend developing each feature on a new branch. You are on the master branch which I would recommend you not to use in the future

This might not be exactly right, but hopefully will work or be pretty close:

git remote add upstream git@github.com:chartjs/Chart.js.git
git fetch upstream
git merge upstream/master (at this point you will have merge conflicts and will need to fix them)
git add . (to mark all merge conflicts as fixed)
git rebase --continue
git push -f

touletan · 2018-07-19T19:05:13Z

After additional testing, I removed the code related to the point rotation, since it was causing issue with the legend symbol position. I need help to figure out how to re-introduce point rotation ....

touletan · 2018-07-20T02:44:35Z

that version is good for review.

benmccann · 2018-08-01T02:45:15Z

@touletan I'm very sorry this PR will need to be rebased again. Would you be able to update it?

benmccann · 2018-08-10T14:27:54Z

Thanks @touletan! And sorry again for the delay on getting this in. We are trying to get through the backlog of open reviews. I'm afraid it's hard for us all to find enough spare time, but hopefully we will make it through them all eventually

benmccann · 2018-08-10T14:38:12Z

src/helpers/helpers.canvas.js

 }
+ var radius = width / 2;
+ var yRadius = height / 2;
+ var padLeft = isPoint ? radius - width / 2 / Math.sqrt(2) : 0;


can you add a comment explaining why padding is necessary and what isPoint is doing? I'm not sure what that part is for

I added comments on that. In the production version some symbols can only be called as point and they are not usign the full width since the size is based on the radius. When is used as a legend, I want to be sure that the symbol width = the width value pass as parameter. So to not impact the current behavior (when symbol is called for a point) I did that logic for the impacted symbols. I.e. Add padding when called as a point (not usign the full width) and use the full width (no padding) when called as a legend symbol.

src/helpers/helpers.canvas.js

docs/configuration/legend.md

src/helpers/helpers.canvas.js

kurkle · 2018-12-15T00:16:24Z

src/plugins/plugin.legend.js

@@ -44,8 +45,13 @@ defaults._set('global', {
 // lineDashOffset :
 // lineJoin :
 // lineWidth :
+ // pointStyle: symbol use on legend when usePointStyle is set
+ // legendSymbol : Symbol use on legend if not usePointStyle
+ // boxWidth : width of legend symbol if not userPointStyle


userPointStyle should be usePointStyle.
I would also reword these to something like:

// pointStyle: symbol used on legend when 'usePointStyle' is true // legendSymbol : symbol used on legend when usePointStyle is false // boxWidth : width of legend symbol when usePointStyle is false

benmccann · 2019-01-05T16:12:23Z

@touletan would you be able to rebase this PR?

benmccann · 2019-04-04T04:27:24Z

@touletan I just wanted to check in one last time to see if you still have an interest in moving forward with this PR. It's been needing to be rebased for quite awhile. We'll close it as inactive if we don't hear back soon though you're always welcome to reopen it in the future when you find time to work on it again

touletan · 2019-04-06T14:48:12Z

I'll have a look.

…

________________________________ De : Ben McCann <notifications@github.com> Envoyé : 4 avril 2019 00:28 À : chartjs/Chart.js Cc : Dominic Jean; Mention Objet : Re: [chartjs/Chart.js] Legend Symbol Parameter [New Feature] (#4811) @touletan<https://github.com/touletan> I just wanted to check in one last time to see if you still have an interest in moving forward with this PR. It's been needing to be rebased for quite awhile. We'll close it as inactive if we don't hear back soon though you're always welcome to reopen it in the future when you find time to work on it again — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#4811 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AE_7QTEeFOwaWyN0dC0-JIjcNoWhfPZXks5vdX9TgaJpZM4Ps-yk>.

benmccann · 2019-05-20T15:28:04Z

@touletan I'm going to close this PR as inactive for the moment to help us keep track of which PRs need review, but please feel free to reopen if you get a chance to rebase and we'll be happy to take a look at it

flaushi · 2019-10-19T19:01:28Z

I'd be in need of this boxHeight option... thanks in advance!

touletan mentioned this pull request Oct 4, 2017

Legend Symbol Parameter [deleted] #4806

Closed

etimberg reviewed Oct 6, 2017

View reviewed changes

etimberg requested a review from simonbrunel October 6, 2017 01:34

Dominic Jean and others added 2 commits October 7, 2017 12:31

Add functionality to display all point style symbols as legend symbol…

1d76be9

…. Symbol type,, boxWidth, border Color and border Width can be defined at dataset level

Update bubble.md

3d85b78

benmccann added this to the Version 2.8 milestone Oct 8, 2017

simonbrunel mentioned this pull request Oct 26, 2017

Add option to allow square legend symbols at different sizes #4890

Closed

simonbrunel modified the milestones: Version 2.7.2, Version 2.8 Jan 13, 2018

rebase and fix merge conflicts

13eabb9

Merge branch 'master' into master

3664607

Merge branch 'master' of git://github.com/chartjs/Chart.js

351fc89

nagix mentioned this pull request Jul 7, 2018

Add legend label style option #5622

Closed

simonbrunel mentioned this pull request Jul 9, 2018

Add "point style" image tests #5629

Merged

merge #4811 with master

aa40651

remove PointRotation. Integration issue with legend .

84e66cf

insert pointRotation functionality back

c54bf70

fix rotated symbol center position.

e8ad16f

jadurani mentioned this pull request Jul 23, 2018

Customize legend box height #2051

Closed

merge with master

40825e8

benmccann added the status: stale label Aug 4, 2018

rebase

8380e75

benmccann removed the status: stale label Aug 10, 2018

benmccann reviewed Aug 10, 2018

View reviewed changes

src/helpers/helpers.canvas.js Outdated Show resolved Hide resolved

benmccann reviewed Aug 10, 2018

View reviewed changes

docs/configuration/legend.md Outdated Show resolved Hide resolved

benmccann added the status: awaiting review label Aug 12, 2018

add/fix comments

3665591

benmccann reviewed Aug 15, 2018

View reviewed changes

src/helpers/helpers.canvas.js Outdated Show resolved Hide resolved

fix comments

6e6fe02

simonbrunel removed this from the Version 2.7.3 milestone Oct 15, 2018

kurkle reviewed Dec 15, 2018

View reviewed changes

kurkle added type: enhancement status: stale and removed status: awaiting review labels Feb 1, 2019

benmccann closed this May 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Legend Symbol Parameter [New Feature] #4811

Legend Symbol Parameter [New Feature] #4811

touletan commented Oct 4, 2017

benmccann commented Oct 4, 2017

etimberg Oct 6, 2017

touletan Oct 6, 2017

etimberg Oct 6, 2017

etimberg Oct 6, 2017

touletan Oct 6, 2017 •

edited

Loading

simonbrunel commented Oct 6, 2017

touletan commented Oct 6, 2017 •

edited

Loading

simonbrunel commented Oct 6, 2017

touletan commented Oct 6, 2017

touletan commented Oct 6, 2017

etimberg commented Oct 7, 2017

touletan commented Oct 7, 2017

etimberg commented Dec 31, 2017

touletan commented Jan 17, 2018

touletan commented Jan 17, 2018

benmccann commented Jul 8, 2018

benmccann commented Jul 15, 2018

touletan commented Jul 17, 2018 via email

benmccann commented Jul 17, 2018

touletan commented Jul 19, 2018

touletan commented Jul 20, 2018

benmccann commented Aug 1, 2018

benmccann commented Aug 10, 2018

benmccann Aug 10, 2018

touletan Aug 15, 2018

kurkle Dec 15, 2018

benmccann commented Jan 5, 2019

benmccann commented Apr 4, 2019

touletan commented Apr 6, 2019 via email

benmccann commented May 20, 2019

flaushi commented Oct 19, 2019

Legend Symbol Parameter [New Feature] #4811

Legend Symbol Parameter [New Feature] #4811

Conversation

touletan commented Oct 4, 2017

benmccann commented Oct 4, 2017

etimberg Oct 6, 2017

Choose a reason for hiding this comment

touletan Oct 6, 2017

Choose a reason for hiding this comment

etimberg Oct 6, 2017

Choose a reason for hiding this comment

etimberg Oct 6, 2017

Choose a reason for hiding this comment

touletan Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

simonbrunel commented Oct 6, 2017

touletan commented Oct 6, 2017 • edited Loading

simonbrunel commented Oct 6, 2017

touletan commented Oct 6, 2017

touletan commented Oct 6, 2017

etimberg commented Oct 7, 2017

touletan commented Oct 7, 2017

etimberg commented Dec 31, 2017

touletan commented Jan 17, 2018

touletan commented Jan 17, 2018

benmccann commented Jul 8, 2018

benmccann commented Jul 15, 2018

touletan commented Jul 17, 2018 via email

benmccann commented Jul 17, 2018

touletan commented Jul 19, 2018

touletan commented Jul 20, 2018

benmccann commented Aug 1, 2018

benmccann commented Aug 10, 2018

benmccann Aug 10, 2018

Choose a reason for hiding this comment

touletan Aug 15, 2018

Choose a reason for hiding this comment

kurkle Dec 15, 2018

Choose a reason for hiding this comment

benmccann commented Jan 5, 2019

benmccann commented Apr 4, 2019

touletan commented Apr 6, 2019 via email

benmccann commented May 20, 2019

flaushi commented Oct 19, 2019

touletan Oct 6, 2017 •

edited

Loading

touletan commented Oct 6, 2017 •

edited

Loading