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

Axis options #53

Merged
merged 47 commits into from
Feb 26, 2019
Merged

Axis options #53

merged 47 commits into from
Feb 26, 2019

Conversation

GraceGSy
Copy link
Contributor

@GraceGSy GraceGSy commented Feb 8, 2019

Changes in this PR:

Axis options

  • Styling options added for axis main line, labels, title and ticks
  • Positioning options for axis using hjust/vjust or x, w/y, h (in screen pixels) or x1, x2/y1, y2 (in screen pixels)
  • An extra tick can be added at the origin of each axis for non-categorical variables, label for this extra tick can be toggled
  • Minor adjustment to defaultFormat.js so that all output is in type String

Nested axes

  • Axes can now be nested in subsections, see Multiline in sandbox

Axis docs

  • Updated axis docs

Feedback needed

  • In order to fix the height/width of the x/y axes to 100px, a recursive function getParents() is used to implement an inverse this.$$transform
  • This may not be the best/most efficient method of converting screen pixels to local coordinates
  • Also: the extra tick comes with a label, that may sometimes be oddly formatted (see scatterplot). This implies that the user will have to manually set the formatting if the label turns out weird. Should we just remove the label for the extra tick at origin?

GraceGSy and others added 30 commits January 16, 2019 14:07
@GraceGSy
Copy link
Contributor Author

Thanks for the feedback! Just to clarify the points raised:

  • I'm using the getParents() function to compute this._parentNodes and then using that for the getLocalX and getLocalY functions. They're used in individual axis components, such as tickMin, tickMax, or converting this.x1 into local coordinates. In general, the width/height dimensions are entered using the inherited Rectangular props (so x1, x2, y1, y2 etc...). But I definitely agree that getParents() is doing extra work, so I don't really like it as a solution. Adding String processing logic to Rectangular sounds much better.

[Also to be honest, there is no real reason the axis needs to be any particular width/height since tick sizes and stroke widths are all customisable. I'm just arbitrarily using 100px to set the size of the section enclosing the axis. Maybe this isn't the best implementation but I wanted to make it easier to calculate local coordinates for tick sizes in some cases.]

  • I think I must have missed some vgg-points when merging. Which ones are still showing up as vgg-symbol? B/c I do agree that using radius with points is more intuitive, so if anything is still using vgg-symbol that would be unintentional...

  • Yeah, an extra prop for first tick label sounds good

@luucvanderzee
Copy link
Collaborator

Wait, if the axis is just inheriting from the section that it is in, why does the section need to be 100px? I don't really understand this point

@vercel vercel bot temporarily deployed to staging February 11, 2019 03:33 Inactive
docs/axes/cartesian.md Outdated Show resolved Hide resolved
src/components/Core/Section.vue Show resolved Hide resolved
src/components/Guides/YAxis.vue Show resolved Hide resolved
src/mixins/Guides/BaseAxis.js Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to staging February 26, 2019 09:32 Inactive
@vercel vercel bot temporarily deployed to staging February 26, 2019 09:57 Inactive
@luucvanderzee luucvanderzee merged commit d54380f into master Feb 26, 2019
@luucvanderzee luucvanderzee deleted the axis-options branch February 26, 2019 10:14
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.

3 participants