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

feat(coordinateGridMixin): useTopXAxis #1816

Closed
wants to merge 7 commits into from
Closed

feat(coordinateGridMixin): useTopXAxis #1816

wants to merge 7 commits into from

Conversation

aberenyi
Copy link
Contributor

@gordonwoodhull could you please cast your eye over this?

Copy link
Contributor

@gordonwoodhull gordonwoodhull left a comment

Choose a reason for hiding this comment

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

Thanks @aberenyi! Overall the changes look great.

As a matter of policy, we usually don't try to adjust margins automatically. The interaction of features gets really complicated and we would have to devise a strategy (perhaps using flexbox) rather than writing special case logic everywhere.

So for now we leave this in control of the user. In this case it's better to change the example.

Also I found the double ?: confusing so I made a suggestion there.

src/base/coordinate-grid-mixin.js Outdated Show resolved Hide resolved
web-src/examples/scatter-top.html Show resolved Hide resolved
src/base/coordinate-grid-mixin.js Outdated Show resolved Hide resolved
web-src/examples/scatter-top.html Outdated Show resolved Hide resolved
@aberenyi aberenyi requested a review from gordonwoodhull March 9, 2021 16:34
@gordonwoodhull
Copy link
Contributor

Rebased and merged; released in 4.2.5

@gordonwoodhull
Copy link
Contributor

Thanks @aberenyi!

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