-
Notifications
You must be signed in to change notification settings - Fork 113
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
Added the sticky option to center Axes #295
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution, for the first review I left a comment about the API design.
A few other notes:
- Please add a snapshot test in https://github.com/mauriciopoppe/function-plot/blob/master/test/e2e/snippets.ts
- Please run the code formatter tool https://prettier.io/docs/en/cli to keep the same code style.
site/js/site.js
Outdated
@@ -95,6 +95,7 @@ function onSiteDependenciesLoaded() { | |||
* - `domain`: x axis possible values (see examples below) | |||
* - `yAxis`: same options as `xAxis` | |||
* - `disableZoom`: true to disable translation/scaling on the graph | |||
* - `centerAxes`: true to center the axes in the middle of the graph |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment about API design, what about making this an option of the xAxis, yAxis fields like position: 'sticky'
e.g.
xAxis: {
label: 'x - axis',
position: 'sticky',
},
yAxis: {
label: 'y - axis'
position: 'sticky',
},
- Why
sticky
? Seems like a value that matches what the position field in CSS https://developer.mozilla.org/en-US/docs/Web/CSS/position - What should be the default value for position? Given that this is a new option it should have a default that's backward compatible, possible values could be
['bottom', 'left', 'sticky']
with the defaults:
xAxis: {
label: 'x - axis',
position: 'bottom',
},
yAxis: {
label: 'y - axis'
position: 'left',
},
site/partials/examples.html
Outdated
@@ -30,6 +30,7 @@ | |||
<li><code>domain</code>: x axis possible values (see examples below)</li> | |||
<li><code>yAxis</code>: same options as <code>xAxis</code></li> | |||
<li><code>disableZoom</code>: true to disable translation/scaling on the graph</li> | |||
<li><code>centerAxes</code>: true to center the axes in the middle of the graph</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is autogenerated, first please address the API design comment and we can come back to this file later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for adding a test and using prettier to format the code! I pulled the changes locally and it looks great, I dragged a graph and I see the axis stick correctly to all the locations and floating when either x=0 or y=0 which is good.
I left more comments, also I noticed that 0 is displayed in both axis, it's a tiny nit but address it if you can.
src/chart.ts
Outdated
canvas | ||
.select('.x.axis') | ||
.call(instance.meta.xAxis) | ||
.attr('transform', 'translate(0,' + yTranslation + ')') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this line in two places:
.select('.x.axis')
.attr('transform', 'translate(0,' + yTranslation + ')')
...
.selectAll('.x.axis path, .x.axis line')
.attr('transform', 'translate(0,' + yTranslation + ')')
Wouldn't the first one automatically apply a translation to the entire group (including the path
and line
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, those are different values. I don't know why, but when I draw the x lines only with yTranslation, it's buggy, and the grid doesn't render correctly.
.attr('transform', 'translate(0,' + (this.meta.height / 2 - yTranslation + this.meta.height / 2) + ')');
I use this for all Paths and Lines
src/chart.ts
Outdated
|
||
canvas | ||
.selectAll('.x.axis path, .x.axis line') | ||
.attr('fill', 'none') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some attr that are reapplied later e.g. line 700:
canvas
.selectAll('.axis path, .axis line')
.attr('fill', 'none')
.attr('stroke', 'black')
.attr('shape-rendering', 'crispedges')
.attr('opacity', 0.1)
Maybe the lines in 700 should be kept and then remove these additional attrs.
I believe this could be introduced as a single commit, please squash all of your commits into one, for more info about squashing commits please read https://kubernetes.io/docs/contribute/new-content/open-a-pr/#squashing-commits. If it's a single commit then it could easily be cherrypicked into the release-1.24 branch or some other commit, the master branch has lots of changes for v2.0 so it'll be harder to create 1.25 out of it but I can create it from the base of |
Now all commits are squashed in one. Can you tell me if I did it right? I did this the first time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last comments, they're mostly nits about how where the variables should exist.
Please also update the title of the PR and the description to match what you did.
Thanks a lot for contributing! I'll add an example in the main webpage about using this option.
src/chart.ts
Outdated
canvas.select('.x.axis').call(instance.meta.xAxis) | ||
|
||
// center the axes | ||
const yMin = this.meta.yScale.domain()[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: All of these variables are only used inside their corresponding ifs, consider moving them to be inside them so that they're properly scoped.
src/types.ts
Outdated
@@ -27,6 +27,19 @@ export interface FunctionPlotOptionsAxis { | |||
* True to invert the direction of the axis | |||
*/ | |||
invert?: boolean | |||
|
|||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Please fix the indentation.
I think you might want to amend the commit so that it describes what the commit is about i.e. Add the sticky positioning to center the origin axes if they're in the viewport. |
b1886b1
to
541d343
Compare
Added the option
position: sticky
to the axis, when set the axis will float in the viewport if the origin is visible.Example:
Ref #291