Skip to content

Conversation

@khanak0509
Copy link
Contributor

@khanak0509 khanak0509 commented Dec 20, 2025

Fixes: #1856
Adds one-time intro modals for inbox and combined feed screens to help new users understand what each view shows.

screenshots -
Screenshot 2025-12-19 at 12 51 27 PM
Screenshot 2025-12-19 at 12 51 44 PM

as mentioned in issue description and CZO discussion: #mobile > Intro video? intro is showing once per install of the app even if they login again . and if they reinstall the add it will visible again .

Tested on iOS emulator:
(1) Fresh install shows both modals on first visit
(2)Modals don't reappear after tapping "Got it"
(3) Modals stay dismissed across app restarts
(4) Reinstalling app shows modals again

video of testing
https://drive.google.com/file/d/1vg3yrCxwh44TSUv6730K7Lr-CPrxI1cm/view?usp=sharing

all tests passed
image

@gnprice
Copy link
Member

gnprice commented Dec 21, 2025

See the project's README:
https://github.com/zulip/zulip-flutter#submitting-a-pull-request

Before we can review this PR, it will need tests.

@khanak0509
Copy link
Contributor Author

now I have added test and also added test detection code in dialog.dart
because the in test mode inbox_test.dart create InboxPage widgets, the modal pops up , When message_list_test.dart creates message list widgets, the modal pops up , and due to this I was getting 45 test failures because it is blocking ui

but when I added test detection it solved
now all tests are passing

@chrisbobbe
Copy link
Collaborator

See the project's README:
https://github.com/zulip/zulip-flutter#submitting-a-pull-request

Before we can review this PR, it will need to be organized into clear and coherent commits, following Zulip's commit style guide.

@khanak0509 khanak0509 force-pushed the intro-modals-1856 branch 2 times, most recently from 9a9c7e3 to b319244 Compare December 24, 2025 18:11
@khanak0509
Copy link
Contributor Author

khanak0509 commented Dec 26, 2025

@gnprice @chrisbobbe now could you please review it ?

@gnprice
Copy link
Member

gnprice commented Jan 6, 2026

This still makes a number of irrelevant changes to other parts of the code.

@khanak0509 Before you ask others to spend time reviewing your work, you need to take the time to review it carefully yourself. See the discussion in our README (linked twice above) and in the Zulip contributing guide linked from there.

@khanak0509
Copy link
Contributor Author

Done with all the changes. Also, in lib/widgets/dialog.dart at line 188, I intentionally made this change and added curly brackets because I was getting ->
image

@gnprice
Copy link
Member

gnprice commented Jan 6, 2026

OK, that's clean enough to be reviewed.

This still needs tests, though. The tests need to check that the app satisfies the main point of the issue: when the user first visits the inbox or combined feed, they get the intro modal, and when they visit again, they don't.

The current version adds some test cases in test/widgets/dialog_test.dart, but these are "unit tests" of the kind that don't really get to the main point.

I gave a talk last year at Fluttercon USA about how to write good tests, including more details and examples about this point. I recommend watching that talk.

@khanak0509
Copy link
Contributor Author

Thanks for sharing this. I watched it and understood the difference between unit tests and integration tests.
I have tried to implement an integration test to check whether the app satisfies the main requirement of the issue when a user visits the inbox or combined feed for the first time, they see the intro modal, and when they visit again, they don’t, as you mentioned.
Could you please let me know whether I’m on the right track or not?

@gnprice
Copy link
Member

gnprice commented Jan 15, 2026

Great, glad that was helpful. This revision is closer: it effectively tests most of the behavior of showInboxIntroModal and showCombinedFeedIntroModal.

From the user's perspective, though, they don't call a function showInboxIntroModal; they visit the inbox page. So what we'd really like is a test that simulates that.

In particular, this version would still pass if the call to showInboxIntroModal were to get accidentally deleted from the InboxPage code. The tests we'd like to have here would correctly fail if that happened.

The other remaining piece for making these into end-to-end, user-oriented tests is that the tests should check the titles on the dialogs. As is, the tests would still pass if the dialogs accidentally had their titles swapped.

@khanak0509
Copy link
Contributor Author

OK, now I understood! . In the inbox test, I should navigate to HomePage() instead of calling showInboxIntroModal() directly, and same for combined feed navigate to MsgListPage() instead of calling the function. Based on this, I've made the changes. now is it fine ?

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.

Add intro modals for main screens

3 participants