zulip
zulip copied to clipboard
Hashchange: Change hash of #all-messages to #feed.
This pr makes hashchange of #all-messages to #feed, also redirecting users from #all-messages
to #feed
along with changing hash of help docs from /all-messages
to /combined-feed
along with it's references.
CZO Discussion
Fixes: #27802.
Screenshots and screen captures:
https://github.com/zulip/zulip/assets/91622060/17683840-e004-4f63-b386-39cfe75490d9
Self-review checklist
- [x] Self-reviewed the changes for clarity and maintainability (variable names, code reuse, readability, etc.).
Communicate decisions, questions, and potential concerns.
- [x] Explains differences from previous plans (e.g., issue description).
- [x] Highlights technical choices and bugs encountered.
- [x] Calls out remaining decisions and concerns.
- [x] Automated tests verify logic where appropriate.
Individual commits are ready for review (see commit discipline).
- [x] Each commit is a coherent idea.
- [x] Commit message(s) explain reasoning and motivation for changes.
Completed manual review and testing of the following:
- [x] Visual appearance of the changes.
- [x] Responsiveness and internationalization.
- [x] Strings and tooltips.
- [x] End-to-end functionality of buttons, interactions and flows.
- [x] Corner cases, error conditions, and easily imagined bugs.
🔍 Existing Issues For Review
Your pull request is modifying functions with the following pre-existing issues:
📄 File: web/src/hotkey.js
Function | Unhandled Issue |
---|---|
process_hotkey |
Error: Caller should pass in a single row. id(src... Event Count: 1 Affected Users: 2 |
Did you find this useful? React with a 👍 or 👎
@zulipbot add "area: left sidebar (ui)" "area: documentation (user)"
Hello @zulip/server-user-docs members, this pull request was labeled with the "area: documentation (user)" label, so you may want to check it out!
@laurynmm Are you up for reviewing this one? I haven't tested.
@nimishmedatwal - Thanks for working on this renaming!
I think it would be good to split these changes into two commits. One for the URL update in the web-app and one for the help center changes.
For the help center changes, you're going to need to add a URL redirect for the renaming of the "All messages" article markdown file. See zerver/lib/url_redirects.py
and some of the pull requests that have updated that for help center changes.
For now, I'm removing the "maintainer review" label. Let me know if you have any questions and ping me again for a review when you've updated for those changes.
@laurynmm I've made all the requested changes, can you give this another review?
@nimishmedatwal You can use Zulipbot to re-add the maintainer review label once this PR passes tests.
@zulipbot add "maintainer review"
@laurynmm Thanks for the thorough review!
I've corrected all the issues you pointed out, also should I change user_settings.web_home_view
in server? For now I've changed it back to all_messages
.
Also I am not sure whether to change this instance of all_messages
in help_relative_link.py
message_info = {
"drafts": ["Drafts", "/#drafts", draft_instructions],
"scheduled": ["Scheduled messages", "/#scheduled", scheduled_instructions],
"recent": ["Recent conversations", "/#recent", recent_instructions],
"all": ["Combined feed", "/#all_messages", all_instructions],
"starred": ["Starred messages", "/#narrow/is/starred", starred_instructions],
"direct": ["Direct message feed", "/#narrow/is/dm", direct_instructions],
"inbox": ["Inbox", "/#inbox", inbox_instructions],
}
I've corrected all the issues you pointed out, also should I change
user_settings.web_home_view
in server?
Yes, but please do that in its own commit, and include a migration to update the value for all existing users and realmuserdefaults. That can possibly be done in a follow-up PR.
For that help_relative_link
detail, that whole section seems like dead code; @drrosa may remember the context (0415e66969840aa450e9c06407921547bd4c3b93 removed the only use of it).
$ git grep 'relative\|message\|' help
help/direct-messages.md:{relative|message|direct}
I went ahead and did the finishing work on this to make sure I was happy with the strings in the comments; with those changes, I think this is good to merge, thanks @nimishmedatwal and @laurynmm! @nimishmedatwal please take more time in your PRs to be careful -- read the surrounding code, run git log
on the files you're changing, and git log -S
on identifiers you don't understand -- and double-check your English writing -- it can really help reduce errors. I also ended up rewriting some of your commit messages.
For follow-ups, we still need an API change and database migration to close this out -- check the history for the "recent topics" rename to see how to do that. And then we also will want to rename variables -- e.g., git grep all_messages_view
finds a function to rename.
@timabbott sure! I will keep that in mind in future.
For follow-ups, we still need an API change and database migration to close this out -- check the history for the "recent topics" rename to see how to do that.
Just a note here that we haven't yet done the API and database migration change for the "recent topics" rename yet, see issue #25781.