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

AutoSkip in update #6355

Merged
merged 5 commits into from
Sep 7, 2019
Merged

AutoSkip in update #6355

merged 5 commits into from
Sep 7, 2019

Conversation

benmccann
Copy link
Contributor

No description provided.

@benmccann benmccann force-pushed the autoskip-update branch 2 times, most recently from 28d3df8 to 90b7a47 Compare June 24, 2019 04:14
src/core/core.scale.js Outdated Show resolved Hide resolved
@benmccann benmccann force-pushed the autoskip-update branch 8 times, most recently from 90d4cea to fed37b6 Compare June 27, 2019 17:02
Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

Commented just some thoughts, nothing really important.

src/core/core.scale.js Show resolved Hide resolved
src/core/core.scale.js Outdated Show resolved Hide resolved
src/core/core.scale.js Outdated Show resolved Hide resolved
src/core/core.scale.js Outdated Show resolved Hide resolved
src/core/core.scale.js Outdated Show resolved Hide resolved
src/core/core.scale.js Outdated Show resolved Hide resolved
Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

I'm still thinking it would be better to let core.layouts set the dimensions after update and do the autoSkip in _configure (function name could maybe be better).

src/core/core.scale.js Outdated Show resolved Hide resolved
@benmccann
Copy link
Contributor Author

benmccann commented Jul 1, 2019

_configure is called twice, which is okay for setting simple properties, but I'm not sure we'd want to call more complicated methods from it, which could be more expensive. Also, it'd give the wrong results if you skip too many ticks the first time it's called unless you also called buildTicks again or stored both the skipped and unskipped ticks

@benmccann
Copy link
Contributor Author

I've rebased this PR

@benmccann
Copy link
Contributor Author

@etimberg @nagix @kurkle any additional thoughts?

src/core/core.scale.js Outdated Show resolved Hide resolved
src/core/core.scale.js Outdated Show resolved Hide resolved
test/specs/core.scale.tests.js Outdated Show resolved Hide resolved
@kurkle
Copy link
Member

kurkle commented Aug 13, 2019

Needs to be rebased. Also I don't like the todo. I'd remove it (or at least add something about v3 so new devs don't waste their time doing it in v2)

@benmccann
Copy link
Contributor Author

Thanks for taking a look. I've added v3 to the todo

kurkle
kurkle previously approved these changes Aug 13, 2019
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
* AutoSkip in update

* Address review comments

* Add v3 to TODO

* Address review comments

* Remove unrelated code cleanup
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.

4 participants