-
-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
Add Not subscribe
tab in channels settings.
#30021
Conversation
Hello @zulip/server-streams members, this pull request was labeled with the "area: stream settings" label, so you may want to check it out! |
41fb128
to
d2764b1
Compare
Not subscribe tab
in channels settings
Not subscribe tab
in channels settingsNot subscribe
tab in **channels settings**.
Not subscribe
tab in **channels settings**.Not subscribe
tab in channels settings.
Thanks! The tooltip should refer to "channels", not "streams" now. |
d2764b1
to
50b9f81
Compare
@alya made the changes :) |
@sahil839 can you review this one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Posted few comments.
Also, the "Fixes .." line in commit message should be in the last commit which completes all the points mentioned in the issue. In the previous commit, you can write "Fixes part of ..".
50b9f81
to
04ae6ed
Compare
@sahil839 made the changes according to your feedbacks kindly review :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Posted one comment. Also, please update the commit message to include "Fixes .." line.
04ae6ed
to
1679d67
Compare
@sahil839 made the changes. kindly review :) |
Looks good to me. @timabbott ready for your review. There are a couple of things - here and here, that I am not sure and need feeback. |
This commit changes the variable and function name from "subscribed_only" to "show_subscribed" and "set_subscribed_only" to "set_show_subscribed" so that in the next commit, when the "not subscribed" tab is added, we can use a similar variable named "show_unsubscribed" and function named "set_show_unsubscribed".
This commit adds a new tab to the stream settings overlay called "Not subscribed" which lists all the streams the user is not subscribed to. This tab is disabled if the user is a guest. Introduced a new variable called "show_not_subscribed" to replicate a similar model to how the subscribed tab is working. Currently, there are only two tabs: "subscribed" and "all streams" so we can use an if-else condition. Refactored the 'update_stream_row_in_settings_tab' function inside stream_ui_updates.js to include the case of a 'Not-subscribed' tab, so that the tab is immediately updated in any add/remove subscription event. Fixed and added the node tests for these changes.
In the "not-subscribed" tab, when there are no channels to show, we display a text message saying "No channels to show. View all channels" along with a link that redirects the user to the "All channels" tab (All channels(#channels/all)). Updated the update_empty_left_panel_message function in stream_settings_ui.js to modify and determine which banner to display when no channels are available to show, modified that function to also display the banner of the "not-subscribed" tab using a new classname called 'not_subscribed_streams_tab_empty_text'.
This commit changes the redirect links for "Browse channels" and "Browse 1 more stream" to the Not subscribed(#streams/notsubscribed) tab.
Render tooltips for both the tabs if the user is guest user. Fixes zulip#21869.
1679d67
to
c94b420
Compare
I forgot to add the "integration review" label before, sorry for that. Added it now. |
Works for me in manual testing! |
case "Not subscribed": | ||
toggler.goto("subscribed"); | ||
break; | ||
case "All channels": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm skeptical of this logic keying off what looks like a translated string; I think this code might not work for non-English user languages.
We should be able to use a data attribute for these conditionals, rather than reading text from the DOM.
This isn't a new issue in this PR, though; can you just fix that in a quick follow-up PR @sujalshah-bit?
Merged, thanks for all the work on this @sujalshah-bit and @sahil839! @sujalshah-bit can you pick up #30021 (comment) as your next priority? It'd be great to fix that while this is fresh, since it seems not very deep. |
PR summary:
Not Subscribed tab
to the channels settings that will contain all the channels the user is not subscribed to.No Channels to show. View all channels.
when no channels is available to show.Browse channel
andBrowse 1 more channels
to theNot subscribed
tab.Not subscribed
tab if the user is a guest user.Fixes: #21869
Continuing the works of: #26049
not-subscribed.mp4
Self-review checklist
(variable names, code reuse, readability, etc.).
Communicate decisions, questions, and potential concerns.
Individual commits are ready for review (see commit discipline).
Completed manual review and testing of the following: