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.

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

message_ui: Show non-sticky recipient bar action buttons on hover. #27265

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ecxtacy
Copy link
Collaborator

@ecxtacy ecxtacy commented Oct 18, 2023

Fixes: #26852

CZO thread (The design idea discussion)
CZO thread (Implementation approach discussion)

  • Recipient bars show the action buttons on hover, except for the sticky recipient bar which always shows the action buttons.
  • The CSS rule visibility: hidden; is toggled on hover, and visibility: visible; applied if the bar is sticky.
  • The disappearance of the action buttons when action UI (notification menu, topic editing UI) is shown is prevented by using :focus-within CSS pseudo class on the recipient_bar to add visibility: visible to the action buttons.
  • In case of conversation views, since there is a single message box present, the controls are made forever visible by using the CSS :only-child pseudo selector.

Screenshots and screen captures:

Final Look

zulip-268

Action UI remains intact
Previously (All controls visible)
Conversation view

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.

@zulipbot zulipbot added size: S area: message feed (UI) Buttons/UI directly in the message feed (not popovers, etc.) priority: high redesign Part of the webapp redesign project, including blockers. UI experiment Design direction to be evaluated labels Oct 18, 2023
@zulipbot
Copy link
Member

Hello @zulip/design members, this pull request was labeled with the "UI experiment", "redesign" labels, so you may want to check it out!

@alya
Copy link
Contributor

alya commented Oct 18, 2023

Thanks!

If a user is editing a topic, the editing UI should always be shown. It should not disappear if the mouse is moved.

Screenshot 2023-10-18 at 11 50 35 AM

@alya
Copy link
Contributor

alya commented Oct 18, 2023

Similarly, the notification settings button should remain visible while the menu is open, avoiding situations like this:

Screenshot 2023-10-18 at 11 51 50 AM

@ecxtacy
Copy link
Collaborator Author

ecxtacy commented Oct 19, 2023

Thanks a lot @alya, for bringing up these flaws, I will work on fixing them. 😃

@ecxtacy ecxtacy force-pushed the issue_26852-hover-action-button branch from 7c5dc6e to 9a9f0c2 Compare October 20, 2023 10:04
@ecxtacy
Copy link
Collaborator Author

ecxtacy commented Oct 20, 2023

@alya I have fixed the flaws which were produced and rebased + updated my branch and updated the PR description. 😄

This is the UI after the fix :-

@alya
Copy link
Contributor

alya commented Oct 24, 2023

The changes in response to the comments above look good! Please also address the comments here -- the icons should always be shown in a view that just has one conversation.

@ecxtacy ecxtacy force-pushed the issue_26852-hover-action-button branch 2 times, most recently from 7f563b4 to 9f61e29 Compare October 25, 2023 18:00
@zulipbot zulipbot added size: M and removed size: S labels Oct 25, 2023
@ecxtacy ecxtacy force-pushed the issue_26852-hover-action-button branch from 9f61e29 to fe4c7aa Compare October 25, 2023 18:01
@ecxtacy
Copy link
Collaborator Author

ecxtacy commented Oct 25, 2023

As discussed on the CZO regarding the actions visibility in conversation view, I have fixed that bug and updated my PR. 😄

@alya
Copy link
Contributor

alya commented Oct 25, 2023

Thanks! Please post a screen capture or screenshot demonstrating the changes.

@ecxtacy
Copy link
Collaborator Author

ecxtacy commented Oct 25, 2023

After the changes :-

@alya
Copy link
Contributor

alya commented Oct 26, 2023

Works for me in manual testing! @amanagr are you up for reviewing this one?

@alya alya added the maintainer review PR is ready for review by Zulip maintainers. label Oct 26, 2023
@amanagr
Copy link
Member

amanagr commented Oct 27, 2023

LGTM!

@amanagr amanagr added integration review Added by maintainers when a PR may be ready for integration. and removed maintainer review PR is ready for review by Zulip maintainers. labels Oct 27, 2023
@ecxtacy ecxtacy force-pushed the issue_26852-hover-action-button branch 2 times, most recently from 8d2463b to 359434e Compare November 1, 2023 11:52
@ecxtacy
Copy link
Collaborator Author

ecxtacy commented Nov 1, 2023

I've updated the code, which addresses the bug mentioned by Tim. Now the action buttons are always visible only in conversation views, not any other view which might have a single message.

@alya
Copy link
Contributor

alya commented Nov 3, 2023

I did another round of manual testing and didn't spot any issues.

@alya alya added chat.zulip.org review Added by maintainers when a PR requires chat.zulip.org testing to proceed. and removed integration review Added by maintainers when a PR may be ready for integration. labels Nov 3, 2023
$("#zfilt").addClass("is-conversation-view");
} else {
$("#zfilt").removeClass("is-conversation-view");
}
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong if we are rerendering message_lists.home, which should not mutate zfilt. Please look at the table_name attribute for what part of the DOM to interact with.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ill make the adjustments and update the code. Thank you for your feedback on this 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a check so that the logic gets called only if the table_name attribute of the message_list is "zflit".
Branch has been updated and pushed.

@ecxtacy ecxtacy force-pushed the issue_26852-hover-action-button branch from 359434e to c699cfe Compare November 17, 2023 17:12
@timabbott
Copy link
Sponsor Member

Test-deploying this on chat.zulip.org.

@ecxtacy
Copy link
Collaborator Author

ecxtacy commented Nov 22, 2023

Thanks ! 😄

@timabbott
Copy link
Sponsor Member

Can you rebase this and then ping me to deploy it in DMs? It needs a bit of a rework because zfilt no longer exists.

@ecxtacy
Copy link
Collaborator Author

ecxtacy commented Jan 26, 2024

Sure 😄

@ecxtacy ecxtacy force-pushed the issue_26852-hover-action-button branch from c699cfe to 921e7d5 Compare January 26, 2024 02:10
@ecxtacy
Copy link
Collaborator Author

ecxtacy commented Jan 26, 2024

@timabbott I've rebased it.

@ecxtacy ecxtacy force-pushed the issue_26852-hover-action-button branch from 921e7d5 to 37c0998 Compare January 29, 2024 16:03
@timabbott
Copy link
Sponsor Member

@ecxtacy zfilt no longer exists in the app, so this code can't possibly work - can you please carefully update that and re-test this logic after doing so? It's important to always test your changes after a big rebase.

@ecxtacy
Copy link
Collaborator Author

ecxtacy commented Feb 14, 2024

Yeah sure, I'll make the suitable changes on this. 👍
Thanks for pointing this out. 😄

@timabbott timabbott removed the chat.zulip.org review Added by maintainers when a PR requires chat.zulip.org testing to proceed. label Feb 16, 2024
When the current view is a conversation view, add the class
`is-conversation-view` to `div.focused-message-list`.
The check is done when `message_list_view` is rendered, which is
in the `render()` function.
- Toggles the CSS rule `visibility: hidden;` on the recipient bar
  controls when the bar is hovered.
- If the recipient bar is sticky, then the controls are always visible.
- In case of conversation views, the recipient actions remain visible.

Fixes part of zulip#26852
Added `visibility` CSS with `:focus-within` pseudo-class to
the recipient bar. This keeps the action button visibility
consistent with the action UIs - topic editing UI and notification
menu.

Fixes zulip#26852
@ecxtacy ecxtacy force-pushed the issue_26852-hover-action-button branch from 37c0998 to ee3434a Compare February 17, 2024 09:59
@ecxtacy ecxtacy changed the title css: Show non-sticky recipient bar action buttons on hover. message_ui: Show non-sticky recipient bar action buttons on hover. Feb 17, 2024
@ecxtacy
Copy link
Collaborator Author

ecxtacy commented Feb 17, 2024

I have made the required changes.

@alya alya added the chat.zulip.org review Added by maintainers when a PR requires chat.zulip.org testing to proceed. label Feb 22, 2024
@zulipbot
Copy link
Member

zulipbot commented May 2, 2024

Heads up @ecxtacy, we just merged some commits that conflict with the changes you 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 upstream/main branch and resolve your pull request's merge conflicts accordingly.

@alya alya added the completion candidate PRs with reviews that may unblock merging label May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: message feed (UI) Buttons/UI directly in the message feed (not popovers, etc.) chat.zulip.org review Added by maintainers when a PR requires chat.zulip.org testing to proceed. completion candidate PRs with reviews that may unblock merging has conflicts priority: high redesign Part of the webapp redesign project, including blockers. size: M UI experiment Design direction to be evaluated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

In non-sticky recipient header bars, show action buttons only on hover
5 participants