Skip to content
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.

Sign up for GitHub

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

Jump to bottom

left sidebar: Add Views label for spectators. #30443

Merged
merged 1 commit into from
Jun 18, 2024
Merged

left sidebar: Add Views label for spectators. #30443

merged 1 commit into from
Jun 18, 2024

Conversation

opmkumar
Copy link
Collaborator

@opmkumar opmkumar commented Jun 16, 2024 •
edited
Loading

This PR adds the Views label in the left side bar for the spectators without it being collapsible.

Fixes #30324

Screenshots and screen captures:

image

Self-review checklist
  • Self-reviewed the changes for clarity and maintainability
    (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

  • Explains differences from previous plans (e.g., issue description).
  • Highlights technical choices and bugs encountered.
  • Calls out remaining decisions and concerns.
  • Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

  • Each commit is a coherent idea.
  • Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

  • Visual appearance of the changes.
  • Responsiveness and internationalization.
  • Strings and tooltips.
  • End-to-end functionality of buttons, interactions and flows.
  • Corner cases, error conditions, and easily imagined bugs.

@opmkumar opmkumar force-pushed the fix/Views-label-added-for-spectators branch from 88dc903 to 0a8b8c4 Compare June 16, 2024 12:11
@opmkumar opmkumar requested a review from timabbott June 17, 2024 18:37
@alya
Copy link
Contributor

alya commented Jun 17, 2024

Works for me in manual testing!

@timabbott please take a look at the reply to your question above.

@alya alya added the integration review Added by maintainers when a PR may be ready for integration. label Jun 17, 2024
@opmkumar opmkumar force-pushed the fix/Views-label-added-for-spectators branch 4 times, most recently from 7af0113 to 59129a4 Compare June 18, 2024 09:35
@opmkumar
Copy link
Collaborator Author

opmkumar commented Jun 18, 2024 •
edited
Loading

@timabbott I have removed the cursor: pointer and added cursor: default to the Views label as you have told based on some conditional styling (whether user is a spectator or not) in the views-label-container in left_sidebar.hbs file.

removed pointer

Also I have changed the comment suggested by you in the ts file.
Kindly review my code and provide me your views on it.

@opmkumar @timabbott
left sidebar: Make Views label visible for spectators.
47d9bfb 
We don't include the toggle functionality, to avoid needing to design
what a collapsed Views thing would look like.

Fixes zulip#30324.
@timabbott timabbott force-pushed the fix/Views-label-added-for-spectators branch from 59129a4 to 47d9bfb Compare June 18, 2024 17:25
@timabbott
Copy link
Sponsor Member

Looks great; I removed a stray newline and rewrote the commit message to have a bit more explanation, and marked this to merge once CI passes. Thanks for the contribution @opmkumar!

@timabbott timabbott enabled auto-merge (rebase) June 18, 2024 17:27
@timabbott timabbott merged commit 30f65e5 into zulip:main Jun 18, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timabbott timabbott Awaiting requested review from timabbott

Assignees
No one assigned
Labels
integration review Added by maintainers when a PR may be ready for integration. size: S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Views label for spectators
4 participants
@opmkumar @alya @timabbott @zulipbot