Skip to content

[WIP] Allow major/minor units#106

Open
JanMarvin wants to merge 1 commit intoardata-fr:masterfrom
JanMarvin:gh_issue_90
Open

[WIP] Allow major/minor units#106
JanMarvin wants to merge 1 commit intoardata-fr:masterfrom
JanMarvin:gh_issue_90

Conversation

@JanMarvin
Copy link
Contributor

idea for major/minor units without API changes
For #90 and #105

@davidgohel
Copy link
Member

Thanks for this PR, it works well

I'd like to suggest a slightly different API approach. Instead of encoding the unit value in the name of the major_tick_mark/minor_tick_mark vector, I could add dedicated major_unit and minor_unit parameters:

chart_ax_x(x, 
  major_tick_mark = "cross",
  major_unit = 0.5, # default to NULL
  minor_tick_mark = "none",
  minor_unit = 0.1 # default to NULL
)

This keeps the tick mark type ("in", "out", "cross", "none") and the unit interval as two separate concerns, which feels more conventional as an API.

What do you think?

@JanMarvin
Copy link
Contributor Author

Perfect, I just didn't want to introduce a break.
Maybe chart_ax_x() and chart_ax_y() could be combined (I only had a quick look, but they both seem to share the same code). Let me know if you want to take this over or if I should give it a try.

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