Skip to content

Conversation

@pmyagkov
Copy link

This is an actual rebase of #39 onto the most recent master.

@pmyagkov pmyagkov changed the title Support { block: 'end' } The most recent support of { block: 'end' } Oct 31, 2017
@pmyagkov pmyagkov mentioned this pull request Oct 31, 2017
@jeremenichelli
Copy link
Collaborator

Thanks a lot for the PR @pmyagkov. Now, there are some things that I really like from this PR, like the behavior and arguments checking, and some others I don't like changes involving personal code style preferences and lots of others misc topics like test site.

Are you or @DanielHeath available to discuss and update this PR after a review?

Thanks in advance to both for your work.

@pmyagkov
Copy link
Author

pmyagkov commented Nov 1, 2017

@jeremenichelli code style is absolutely OK. Just write down your comments, I'll fix those. But I found no tests at all, so this testing approach is better than nothing.

Answering your question, yea, I'll fix your comments. Really want to get this merged.

@DanielHeath
Copy link

My original changeset turned out to be not quite right - on chrome desktop it was possible to get the scrollbar stuck at the top of the page (I suspect this happens if a target element is removed from the DOM while scrolling was in progress).

While this bug could be fixed, I'd also suggest adding a setTimeout that forces step to stop recursing. Without that, the current code will (for instance) break scrolling if your system clock changes by an hour (as happens automatically at daylight saving time / when NTP fixes your clock).

@pmyagkov
Copy link
Author

pmyagkov commented Nov 2, 2017

@DanielHeath do you want to fix this by your own?

@DanielHeath
Copy link

DanielHeath commented Nov 2, 2017 via email

@pmyagkov
Copy link
Author

pmyagkov commented Nov 3, 2017

@jeremenichelli @iamdustan so guys would you leave some review comments to fix here?

@jeremenichelli
Copy link
Collaborator

@pmyagkov we will indeed, right now we are busy and the PR is big, so it might takes us some time.

@soluml
Copy link

soluml commented Nov 30, 2017

Any update on this?

@jeremenichelli
Copy link
Collaborator

@soluml I'm gonna have time soon to review this PR. I think that we'll need a strategy to tackle this specific part of the spec, so I don't expect this getting included soon because of the size of the PR itself and other reasons. Sorry.

}

var ALLOWED_BEHAVIOR_VALUES = [undefined, 'auto', 'instant', 'smooth'];
var ALLOWED_BLOCK_VALUES = [undefined, 'start', 'end'];

Choose a reason for hiding this comment

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

@malliapi
Copy link

@jeremenichelli - any news on updating this PR?

@malliapi
Copy link

@DanielHeath - same as above

@malliapi
Copy link

i was wondering if you guys had the time since you shown interest in the past as it looks like an awesome feature !

@DanielHeath
Copy link

@malliapi feel free to fix it up! I no longer need this.

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.

6 participants