Skip to content

Retention of scroll offset in emoji #1269

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.

Sign up for GitHub

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

Jump to bottom
Open
AshutoshKhadse23 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Retention of scroll offset in emoji #1269

AshutoshKhadse23 wants to merge 1 commit into from

Conversation

@AshutoshKhadse23
Copy link

@AshutoshKhadse23 AshutoshKhadse23 commented Jan 9, 2025

The issue regarding retention of scroll offset in emoji is solve
Resolving issue #1175

issue.1175.mp4

@AshutoshKhadse23
Copy link
Author

AshutoshKhadse23 commented Jan 9, 2025

Hello @chrisbobbe ,
Now it have only changes made to solve the issue ,you can review it.

chrisbobbe
chrisbobbe reviewed Jan 22, 2025
View reviewed changes
Copy link
Collaborator

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

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

Thanks! Small comments from a quick review; also, please change the commit message to match the project style.

@gnprice
Copy link
Member

gnprice commented Feb 6, 2025

Thanks @AshutoshKhadse23 for picking this up!

We're now focusing more strictly on launch issues until the app's launch is ready, and this is a post-launch issue.

I'll leave this PR as an exception where we'll proceed with it anyway, because it looks like it may be possible for the fix to be quite small.

The next step will be for you to revise it to address each of @chrisbobbe's comments above. Then we can give it a new round of review.

Because we're prioritizing launch issues, this PR may wait longer for review than others. If you're interested in contributing to Zulip, I recommend taking a look also at open issues in the upcoming milestones up through M5 Launch.

@gnprice gnprice added this to the M6: Post-launch milestone Feb 6, 2025
@gnprice gnprice added the maintainer review PR ready for review by Zulip maintainers label Feb 6, 2025
@gnprice
Copy link
Member

gnprice commented Mar 14, 2025

Cross-linking because I happened across it while scanning through PRs during triage: it looks like a previous iteration of this PR was

@AshutoshKhadse23 as a reminder of comments you received earlier at #1232 (comment) and #1232 (comment): when revising a PR, please update the existing PR rather than sending a new one.

(In any case, for this PR the next step is for you to revise it as described above.)

@AshutoshKhadse23
Copy link
Author

AshutoshKhadse23 commented Apr 1, 2025

Hello @chrisbobbe,
Now it ready for review and I had revised my code to remove the confusion of .toString

@gnprice
Copy link
Member

gnprice commented May 10, 2025

Before we can review this, it will need to meet the two basic requirements described in our README:
https://github.com/zulip/zulip-flutter?tab=readme-ov-file#submitting-a-pull-request

It needs tests, and it needs to be organized in clear and coherent commits.

@gnprice gnprice removed the request for review from chrisbobbe May 10, 2025 01:13
@AshutoshKhadse23 AshutoshKhadse23 force-pushed the retention-scroll-cleaned branch from 77bb431 to bad7df0 Compare January 14, 2026 03:46
@AshutoshKhadse23
Copy link
Author

AshutoshKhadse23 commented Jan 14, 2026

@gnprice @chrisbobbe Sorry for the delay, but now it is ready for review.

@AshutoshKhadse23 AshutoshKhadse23 force-pushed the retention-scroll-cleaned branch from bad7df0 to bf35b81 Compare January 14, 2026 14:06
@gnprice
Copy link
Member

gnprice commented Jan 15, 2026

OK, this looks ready for review, thanks.

@AshutoshKhadse23 I see you have several other PRs open too: #2058, #2073, #2075. Please stick to just one issue until your first PR is merged. See the contributing guide:
https://zulip.readthedocs.io/en/latest/contributing/contributing.html#claiming-an-issue

So please pick one of these four PRs to continue with, and close the other three for now. You can always reopen them later if the corresponding issues are still open.

@AshutoshKhadse23
Copy link
Author

AshutoshKhadse23 commented Jan 15, 2026

@gnprice @chrisbobbe,
I think this PR will not take much time to merge, according to me.
So, could you start reviewing this PR? Until then, I will convert the other PR into drafts

chrisbobbe
chrisbobbe reviewed Jan 15, 2026
View reviewed changes
Copy link
Collaborator

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

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

The commit message refers to #1121, which is the wrong issue. That's confusing; please fix that.

See also one comment below.

test/widgets/autocomplete_test.dart Outdated
Comment on lines 547 to 548
// Scroll down using the controller directly.
scrollController.jumpTo(50);
Copy link
Collaborator

@chrisbobbe chrisbobbe Jan 15, 2026

Choose a reason for hiding this comment

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

Can you use tester.scrollUntilVisible instead of this? See Greg's talk "Writing better tests with less work", in particular the part about testing at "stable interfaces": https://www.youtube.com/watch?v=nf_XYpfsZNg

Copy link
Author

@AshutoshKhadse23 AshutoshKhadse23 Jan 16, 2026

Choose a reason for hiding this comment

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

Can you use tester.scrollUntilVisible instead of this? See Greg's talk "Writing better tests with less work", in particular the part about testing at "stable interfaces": https://www.youtube.com/watch?v=nf_XYpfsZNg

I added this but I am not able to understand why CI is failing as my fork main branch latest commit is failing I have also mention with screenshot in CZO.

Copy link
Author

@AshutoshKhadse23 AshutoshKhadse23 Jan 16, 2026

Choose a reason for hiding this comment

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

This is mentioned in #2082
If you want, I can remove the @OverRide of the CI. It will run successfully, I tested that.

@chrisbobbe
Copy link
Collaborator

chrisbobbe commented Jan 15, 2026

Until then, I will convert the other PR into drafts

Also: I see you have not done this; please do it.

@AshutoshKhadse23 AshutoshKhadse23 force-pushed the retention-scroll-cleaned branch 2 times, most recently from 26d3ff6 to 8ff046b Compare January 16, 2026 05:01
@AshutoshKhadse23
autocomplete: Reset scroll position when query changes
1f2e2b7 
When searching for an emoji (or other autocomplete items), scrolling
through results, and then modifying the query, the autocomplete list
would retain its scrolled position while showing new results. This
created confusing UX where results appeared cut off at the top.

Fix this by adding a ScrollController to the autocomplete ListView
and resetting the scroll position to the top whenever the query
content changes.

Fixes: zulip#1175
@AshutoshKhadse23 AshutoshKhadse23 force-pushed the retention-scroll-cleaned branch from 8ff046b to 1f2e2b7 Compare January 16, 2026 07:49
@AshutoshKhadse23
Copy link
Author

AshutoshKhadse23 commented Jan 16, 2026

Until then, I will convert the other PR into drafts

Also: I see you have not done this; please do it.

I did this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chrisbobbe chrisbobbe Awaiting requested review from chrisbobbe

Assignees

No one assigned

Labels

maintainer review PR ready for review by Zulip maintainers

Projects

None yet

Milestone

MXA: Later

Development

Successfully merging this pull request may close these issues.

3 participants

@AshutoshKhadse23 @gnprice @chrisbobbe