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

Smoother navigation between classes when navigating up from player #95

Closed
wants to merge 3 commits into from

Conversation

mosheduminer
Copy link

This PR addresses item (1) in #93.
It implements that whenever the content list is navigated "up" to from the player, the list is scrolled so the currently playing class is second to top.
In order to properly initialize the ListView at the appropriate position, it was necessary to replace the ListView with the (otherwise identical) ScrollablePositionedList from the scrollable_positioned_list package. (It was actually not possible to initialize it at the correct position, as noted in the comments, but rather the correct position is "jumped" to immediately. This unfortunately is not great UX).
Feel free to reject or modify this PR 👍.

@yringler
Copy link
Owner

Wow, very nice! I invited you to be a collaborator, my usual practice during a code review if I want to make a change is to open a PR into the PR, to allow to see clearly what changes I'm making. IYH in the next week - next I expect to be back into my regular hours for the app.

Offhand, you should be able to use - I think a scrollcontroller? I landed a PR with such a thing in flutter_breadcrumbs (used in this app). It had to run outside of the current build cycle, as you saw. That would probably also allow smoother transition, but I think being jumped is also fine.
I figure a person knows what class he was just listening to, and will be more oriented jumped to the middle of the lost than dropped at the top.

This was a feature request, via email or whatsapp or something, if I recall correctly.

@yringler
Copy link
Owner

I left a review, LMK if you have any questions.
In general, it's best not to open a PR branch from master - what if I add other commits to master, and you want to open a PR? It's simpler if your master is up to date with the main master, and you have a branch like 93-smooth-scroll

@mosheduminer
Copy link
Author

100% about the PR from master comment - I'm used to simply creating a branch in the same repo, hence the oversight.

Sorry about not following up on your comments, I had a crazy week last week, and this just fell completely forgotten by the wayside. I'll review and respond soon.

@yringler
Copy link
Owner

100% about the PR from master comment - I'm used to simply creating a branch in the same repo, hence the oversight.

Also, you should have permissions now to create a branch and use the main repository, if you accept my collaborator invitation.

Sorry about not following up on your comments, I had a crazy week last week, and this just fell completely forgotten by the wayside. I'll review and respond soon.

That's fine. It doesn't disturb any of my other work on the app if this takes longer or shorter; this is a pretty stand alone PR.

@mosheduminer
Copy link
Author

Hey @yringler, I know I dropped off the grid for a while, but I'd like to fix the merge conflicts and get this merged. Also, if you're still interested in my help in the future, I can start working on this more.

Also, you should have permissions now to create a branch and use the main repository, if you accept my collaborator invitation.

I think you gave me member access to InsideChassidus-org, but not to this repository.

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