Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#17724 closed enhancement (fixed)

[Patch][CLA] Add dojox.chart centered axis

Reported by: youngho Owned by: Eric Durocher
Priority: high Milestone: 1.10
Component: Charting Version: 1.9.2
Keywords: Cc: dylan
Blocked By: Blocking:

Description

Hello,

I hope to add cented axis for math graph which generated like dojox.calc.Grapher.

Attachments (4)

CenterAxis.patch (7.4 KB) - added by youngho 6 years ago.
CenterAxis.png (14.5 KB) - added by youngho 6 years ago.
math graph drawing with centered axis
rotatedLabels.png (59.1 KB) - added by youngho 5 years ago.
test_rotatedLabels.html.patch (3.1 KB) - added by youngho 5 years ago.

Download all attachments as: .zip

Change History (17)

Changed 6 years ago by youngho

Attachment: CenterAxis.patch added

Changed 6 years ago by youngho

Attachment: CenterAxis.png added

math graph drawing with centered axis

comment:1 Changed 6 years ago by dylan

Milestone: tbd1.10
Owner: set to Eric Durocher
Priority: undecidedhigh
Status: newassigned

comment:2 Changed 6 years ago by Eric Durocher

Hi @youngho, this looks nice, the only thing I would say is that maybe it would be better to use strings like "leftOrBottom", "center" and "rightOrTop" (or shorter names if you have ideas) instead of -1/0/1 for the position value.

Could you please create a pull request on the the GitHub? dojo/dojox repository (see https://github.com/dojo/dojox/blob/master/CONTRIBUTING.md), so it is easier to discuss and track?

Thanks.

comment:3 Changed 5 years ago by Eric Durocher

Cc: dylan added

Another question is whether it would be much more complicated to allow any position, e.g. position could be a 0-1 float? Maybe more complicated due to limit cases, like axis close to the left side so labels must be taken care of...

Last edited 5 years ago by Eric Durocher (previous) (diff)

comment:4 Changed 5 years ago by youngho

Hello Eric,

Sorry to late respons.

Looks great! Please go ahead.

comment:5 in reply to:  4 Changed 5 years ago by Eric Durocher

Hi,

Sorry but what are you agreeing with? Replacing -1/0/1 with string values as in my first comment? Or allowing any position as in my second comment? The second proposal would probably require more time to implement and test than what we have (one day remaining before code freeze). But on the other hand, if we do change this code, it seems a pity to limit the option to just centered axis...

Also, as I said it would have been easier to comment the code in a Github pull request. In particular, I think some of your code changes in getOffsets() are useless since you set the offsets to 0 later.

comment:6 Changed 5 years ago by youngho

Hi Eric,

I was tring to set up a pull request. But because I am not familliar with github, I need a learning time. please understand me.

For the issue,

I think it is better to seperate between axis position notation and how far seperate the label positioned from axis.

for the axis position notation, my preference is -1, 0, +1 notaion. we can easily extends to other enhancement for example, please look at the #17711 we can set this kinds of labels as [-1, +1, +2]

Thanks,

Youngho

comment:7 in reply to:  6 Changed 5 years ago by Eric Durocher

Replying to youngho:

Hi Eric,

I was tring to set up a pull request. But because I am not familliar with github, I need a learning time. please understand me.

Sure I understand.

For the issue,

I think it is better to seperate between axis position notation and how far seperate the label positioned from axis.

I don't understand this, I don't think I spoke about something like that...

for the axis position notation, my preference is -1, 0, +1 notaion.

Well as I said I am sure we can propose something easier to understand for the user.

we can easily extends to other enhancement for example, please look at the #17711 we can set this kinds of labels as [-1, +1, +2]

I don't understand, but that seems another story?

Anyway, all in all I don't think this is quite ready for committing it in 1.10. I would really like to add axis positioning, but it seems we still need discussions on how exactly to do it.

In addition, I think this needs more testing, in particular unit tests to check that it does not break anything together with things like rotated labels, etc.

comment:8 Changed 5 years ago by Eric Durocher

Milestone: 1.101.11

As noted in the previous comment, I think this must be discussed more and reworked, and is not ready for committing in 1.10 as is. So I am moving this to the 1.11 milestone.

Changed 5 years ago by youngho

Attachment: rotatedLabels.png added

Changed 5 years ago by youngho

comment:9 Changed 5 years ago by youngho

Hi Eric,

I added rotatedLabel test case patch.

Also please let me know there any potential breakable current feature exist from my patch.

And I didn't catch why you think about float value at the second your comment. can you explain more ?

comment:10 Changed 5 years ago by Eric Durocher

Thanks for the test case for rotated labels.

About my second comment/float value, my point is that I think it would be useful to be able to place the axis not only at the center, but at any position like 1/4 of the width etc. For this, we would allow the position to be a float between 0 and 1; assuming a vertical axis, the values would mean 0 = left, 1 = right, 0.5 = center, 0.25 = 1/4 of the chart width, 0.75 = 3/4 of the width, etc.

So, what I would like to avoid is changing the API and deprecate a property, and then having to change again because the first change was not sufficient or general enough.

Is it clearer?

comment:11 Changed 5 years ago by Eric Durocher

What we can do is do only centered axis for now, but change -1/0/+1 (which is my opinion are too misleading) to string values "leftOrBottom"/"center"/"rightOrTop". This way, we can decide later to also allow arbitrary float values as well, without breaking compatibility. If you are OK with that and you can provide a patch today, I can commit that for 1.10.

Thanks!

comment:12 Changed 5 years ago by Eric Durocher

Milestone: 1.111.10
Resolution: fixed
Status: assignedclosed

OK, never mind I did it myself.

Committed in: https://github.com/dojo/dojox/commit/6c2f3c03c5adb24d36d97d2cbeb15cf1cd04fd99

Thanks!

comment:13 Changed 5 years ago by Eric Durocher

I also updated the release notes: http://livedocs.dojotoolkit.org/releasenotes/1.10#charting, please review.

If you could update the ref doc http://livedocs.dojotoolkit.org/dojox/charting#configuring-axis I would appreciate, thanks.

Note: See TracTickets for help on using tickets.