-
Notifications
You must be signed in to change notification settings - Fork 309
message-list : add a prompt to notify guest(s) when DM #1362
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
Conversation
Thanks! Marking as draft since this won't be ready for review until it has tests. For help writing tests, the best place to ask is in Zulip in #mobile-dev-help. |
Fixes zulip#798 showDMWarningBanner could be initialized in the initState to shouldShowGuestUserWarningPrompt(store) and Visibility wrapper could have been gotten rid off , but due to unavailability of store beforehand it was not possible. To create the guests list, I got rid off all the users in the store which are not in the recepients list and role-checked them.
9dbe3e0
to
25c4671
Compare
@gnprice I think the PR is ready for review. Let me know if I need to do some changes beforehand. |
I'm seeing a lot of changes that don't look related to the work that's needed; please remove those. Also, CI is failing; you'll need to fix that. |
@chrisbobbe it will help a lot if you can mention the changes that seem not required, either here or I can create a topic in mobile-dev-help channel. |
For example I see many changes in |
25c4671
to
0dcbc89
Compare
@chrisbobbe unnecessary fixes removed and CI checks passed. |
As I wrote in January (#1143 (comment)), in order for us to spend time reviewing your work, you need to do a better job of self-reviewing it first. The comments above are all things you should have spotted in self-review. This revision is better than the first one, but still doesn't meet our standards for clear commits (which are linked in our README and in several comments on #1143 from December): among other things there are still unrelated changes (though fewer of them), and the tests are separated from the code they test. The logic of the implementation also needs a lot of changes before it's something we can merge. Among other things:
So @Abhisheksainii based on #1143 and now this thread, I think you don't yet have the skills to learn to contribute productively in the Zulip project. You're welcome to try again after at least 6 months. I recommend you spend time working on other projects, and reading a lot of code, especially reading code in well-maintained codebases like Zulip or upstream Flutter (i.e., any of the main repositories at https://github.com/zulip or https://github.com/flutter). This advice from our contributing guide is applicable to a wide variety of codebases:
I recommend also investing effort in clearly explaining why your changes work correctly. As our guide to reviewable PRs says, that begins with clearly reasoning through that question yourself:
Even in personal projects where nobody else is reading your explanations, taking the time to clearly write them down will help you write higher-quality code. It will also be useful practice for contributing to projects like Zulip where there are other people you need to explain your changes to. |
No problem, Thanks for the guidance. I will make sure that I work up on my ability to read the code and self-review it. |
Fixes #798
Changes made:
Add a prompt above composeBox to notify the user if the recipient is a guest or not. Attaching screenshots and video for reference.
WhatsApp.Video.2025-02-16.at.13.08.40.mp4