Skip to content

Conversation

@abarth
Copy link
Contributor

@abarth abarth commented May 2, 2017

No description provided.

@abarth abarth requested review from HansMuller and Hixie May 2, 2017 00:15
Copy link
Contributor

Choose a reason for hiding this comment

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

s/State/ScrollController/

Copy link
Contributor

Choose a reason for hiding this comment

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

we really should factor out these docs into an interface at the foundation layer.

Copy link
Contributor

Choose a reason for hiding this comment

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

some of these should be plural, e.g. zero or more [UserScrollNotification]s

Copy link
Contributor

Choose a reason for hiding this comment

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

actually this will fire when it hasn't changed its scroll position, but would have. Basically it's what translates into a glow. On iOS this never fires.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good to know. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

this description of OverscrollNotification is incorrect, I believe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

"the sooner it stops" is probably less odd-sounding that "the faster it slows down". :-)

@Hixie
Copy link
Contributor

Hixie commented May 2, 2017

LGTM

Copy link
Contributor

Choose a reason for hiding this comment

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

I think modeling is the American spelling

Copy link
Contributor

@HansMuller HansMuller May 2, 2017

Choose a reason for hiding this comment

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

that => that a

[Here and elsewhere]

Copy link
Contributor

@HansMuller HansMuller May 2, 2017

Choose a reason for hiding this comment

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

It would be helpful to explain when the dragDetails are non-null, i.e. the drag gesture ended with a velocity that was too small to trigger a ballistic scroll.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor

Choose a reason for hiding this comment

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

ot => to

Copy link
Contributor

Choose a reason for hiding this comment

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

The docs for particle/position/velocity talk about a "particle". We should probably explain the particle angle here or define those methods in terms of the simulation's position; and then mention that position is usually mapped to scroll offset.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe call attention to of() and ensureVisible() here, since those methods will not appear with ListView et al.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear what this means. The property is used to map from input gestures to scroll offset deltas?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe see-also ensureVisible()

@HansMuller
Copy link
Contributor

LGTM too, just some nits.

@abarth
Copy link
Contributor Author

abarth commented May 2, 2017

Thanks!

@abarth abarth merged commit 49d32d3 into flutter:master May 2, 2017
@abarth abarth deleted the moar_docs7 branch May 2, 2017 17:46
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants