views: Updated Stream Info. #856
Conversation
|
@zulipbot add "PR needs review" |
There was a problem hiding this comment.
@zee-bit Thanks for working on this - it opens up a path for showing more stream data, which is great 👍
This functions fairly well and is a great starting point, and I've left some feedback on mainly the code (not tests) inline.
Right now the commit structure is a little confusing - ideally each commit would add a specific element or a particular change, and code associated with that change would be only in that commit. It seems like your idea is that the first commit adds stream members and the second adds the weekly counts, but the first one is named like the PR rather than focusing on the specific change. There are also changes in each commit that are changed from one to the other (eg. stream counts are added as a fixed value in the first then added properly in the second, and the stream member link is named differently in the second weekly-count commit). It would make it easier to follow, review and ultimately merge if changes are isolated cleanly into their own commits :)
As I note inline, the stream members popup may be superseded later by filtering of the user list, which will provide native support for things like searching/filtering for users, so this could be useful in the interim but I think we shouldn't put too much effort into it.
Once these commits are firmed up (or the PR merged), note that there are other stream properties which we could list, based on other available stream data.
zee-bit
force-pushed
the
updateStreamInfo
branch
from
684b11c
to
d21f8ea
Compare
|
@neiljp Sorry, for messing up the whole commit history and structure(I had a very embarrassing perception about PR's and commit's that led to this mess 😅 ). There is another PR where I might have done this same mistake(I'll fix that too). Now, I have updated the code keeping in mind all the reviews above. I have used git interactive-rebase feature to amend these mistakes. The commits are now much cleaner and separate from each other. Hoping to get further review soon. 😊 |
zee-bit
force-pushed
the
updateStreamInfo
branch
2 times, most recently
from
2b12355
to
ac6eea5
Compare
|
@zee-bit I have many PRs to review, but you seem to have the commit structure better, so if you want to update any other PRs like this that'd be great 👍 |
zee-bit
force-pushed
the
updateStreamInfo
branch
from
ac6eea5
to
09760ae
Compare
This commit adds Stream Members view class, that displays the list of all the members subscribed to the stream from the Stream Info view on pressing m key. It also shows the total number of people subscribed in the Stream Info view. Tests amended. Fixes zulip#753.
zee-bit
force-pushed
the
updateStreamInfo
branch
from
09760ae
to
3294bfd
Compare
This is the second commit of this PR, which adds the option of weekly message count for streams from the Stream info view. The stream info is now updated to contain the option to view the list of members subscribed to a stream along with the weekly count of messages in stream. These statistics help get a better overview of the streams and acts as a feature enhancement. Tests amended.
zee-bit
force-pushed
the
updateStreamInfo
branch
from
3294bfd
to
393b582
Compare
|
Heads up @zee-bit, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the |
This commit fixes the bug in StreamMembersView that prevented closing the current and super popup with super().keypress from within StreamMembersView. Follow-up bugfix for #856.
This PR is divided into two parts:
mkey.Tests amended.
This PR fixes #753.