Skip to content

Conversation

@yyonghe
Copy link

@yyonghe yyonghe commented Jun 9, 2020

Location Y axis to right.

@vdobler
Copy link
Owner

vdobler commented Jun 10, 2020

Thanks for this feature!
Some things require cleanup:

  1. Please do not work on the fork, e.g. commit f1dd9ba cannot be merged as it would break the package. Please clone the repo, and do your work on the clone (not on the fork!). Like this you do not have to adapt the import paths. To create a merge request: Add your fork as a new remote to your clone of the repo and push to that fork-remote and create a MR from there.

  2. I would appreciate if the commit message was much more detailed and used the standard format, see e.g. https://gist.github.com/robertpainsi/b632364184e70900af4ab688decf6f53

From reading the commit message and the code it seems as if this change allows to put the axis "on the other" side (right side of plot for Y-axis and top of plot for the X-axis). Is this correct?

Data2Screen func(float64) int // Function to map data value to screen position
Screen2Data func(int) float64 // Inverse of Data2Screen

AlignTrans bool // alignment translation of the axis: false: axis location left/bottom, true: axis location right/top
Copy link
Owner

Choose a reason for hiding this comment

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

AlignTrans is not a clear name. I would prefer something like SwitchSide or AlternateSide or something else but it is not just a translation, and not really an alignment.

For the comment: Start with a capital letter.

Maybe

SwitchSide bool // Switch location of axis to top / right if true.

// Plot outputs the scatter chart to the graphic output g.
func (c *ScatterChart) Plot(g Graphics) {
layout := layout(g, c.Title, c.XRange.Label, c.YRange.Label,
layout := layoutWithAlign(g, c.Title, c.XRange.Label, c.YRange.Label,
Copy link
Owner

Choose a reason for hiding this comment

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

Why is scatterplot the only one which calls layoutWithAlign?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants