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

new conversation: Show correct tooltip when dms are disabled. #30132

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kennethnrk
Copy link
Collaborator

@kennethnrk kennethnrk commented May 18, 2024

Earlier when dms were disabled, the new conversation button tooltip wasn't being updated, therefore a stale tooltip was being shown.

Earlier the data-conversation-type attribute of the new conversation button was being set to direct only if dms were enabled. As a result a stale tooltip was being shown when dms were disabled.
This commit updates the attribute to direct reagardless of dms being enabled or not.

Note

A user can always DM themselves or a bot, even when direct messages are disabled in an org — the reason why we don't disable the "Start new conversation" button. (#28412)

Fixes #29916.

Screenshots and screencaptures

image

336899046-5808aa3a-d1ea-4f41-b03f-3c5468cc144b.webm

@zulipbot zulipbot added size: S area: compose (UI & widgets) Scheduled messages, giphy, send later, etc. area: popovers Popovers, including general message actions. bug labels May 18, 2024
@alya alya added the buddy review GSoC buddy review needed. label Jun 3, 2024
@alya
Copy link
Contributor

alya commented Jun 3, 2024

Since this PR hasn't been reviewed yet, please ping your buddy to take a look.

@kennethnrk
Copy link
Collaborator Author

@PieterCK Please review this PR.

@zulipbot
Copy link
Member

zulipbot commented Jun 4, 2024

ERROR: Label "buddy review" already exists and was thus not added to this pull request.

@PieterCK
Copy link
Collaborator

PieterCK commented Jun 5, 2024

Hi Kenneth, nice work here. code LGTM, but left a suggestion.

Other than that, the PR description could probably be clearer if you add a clip of the new behavior though, so here's one I took when testing. Feel free to add it to the PR description!

Screencast.from.05-06-24.22.10.26.webm

Let me know if you have any questions 👍


Manually tested and found that the expected result described in #29916 has been achieved.
steps: disable DM > go to stream > show tool tip > go to DM > show tool tip

@kennethnrk
Copy link
Collaborator Author

Thanks @PieterCK, I'm not sure how to download and reupload your screen capture to the description, could you please help me. : )

@alya
Copy link
Contributor

alya commented Jun 5, 2024

Thanks for the review, @PieterCK ! When the PR looks good to you, you can remove the "buddy review" label via Zulipbot.

@kennethnrk you can then add a "mentor review" label and ping your mentor.

@PieterCK
Copy link
Collaborator

PieterCK commented Jun 6, 2024

@kennethnrk I think you can right-click on my clip, choose "Save Video As ..." and then it should download. I'm using Firefox though, do you have that functionality in your browser?

@zulipbot remove "buddy review"

@zulipbot zulipbot removed the buddy review GSoC buddy review needed. label Jun 6, 2024
@PieterCK
Copy link
Collaborator

PieterCK commented Jun 6, 2024

Oh, wait. I think you want to add a commit description explaining the reasoning behind the change and why it fixes the issue and better than the current state. Probably something like:
"The previous logic assumes that .... This commit changes .... which improves ....."

For example, here's a good one from Tim! It's from one of your previous PR 👍
ba92cd8
@kennethnrk

@kennethnrk
Copy link
Collaborator Author

@kennethnrk I think you can right-click on my clip, choose "Save Video As ..." and then it should download. I'm using Firefox though, do you have that functionality in your browser?

I'm using Brave, when i clicked save video it was giving me a message media not available. It worked finally. 👍

@kennethnrk
Copy link
Collaborator Author

Thanks @PieterCK, I've made the changes.

@kennethnrk
Copy link
Collaborator Author

@zulipbot add "maintainer review", "mentor review"

@arpit551 Please review this whenever you're available.

@zulipbot zulipbot added the maintainer review PR is ready for review by Zulip maintainers. label Jun 6, 2024
@kennethnrk
Copy link
Collaborator Author

@zulipbot add "mentor review"

@zulipbot zulipbot added the mentor review GSoC mentor review needed. label Jun 6, 2024
@timabbott
Copy link
Sponsor Member

It looks like you added the details that belong in the commit message in the PR description. Since it's commits, not PRs, that get merged, you want 3f4bd50 to have the context for posterity.

@timabbott
Copy link
Sponsor Member

Earlier when dms were disabled, the new conversation button tooltip wasn't being updated, therefore a stale tooltip was being shown.

The previous logic didn't update the new conversation tooltip when dms were disabled. This commit updates the tooltip regardless of dms being disabled.

And then I want to note that this text doesn't answer the key question as to how the old logic failed to do this update; it's not obvious to me from skimming.

Earlier the `data-conversation-type` attribute of the new conversation
button was being set to `direct` only if dms were enabled. As a result a
stale tooltip was being shown when dms were disabled.
This commit updates the attribute to `direct` reagardless of dms being
enabled or not.

Fixes zulip#29916.
@kennethnrk
Copy link
Collaborator Author

It looks like you added the details that belong in the commit message in the PR description. Since it's commits, not PRs, that get merged, you want 3f4bd50 to have the context for posterity.

I'm not sure how I overlooked that. I have updated the commit message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: compose (UI & widgets) Scheduled messages, giphy, send later, etc. area: popovers Popovers, including general message actions. bug maintainer review PR is ready for review by Zulip maintainers. mentor review GSoC mentor review needed. size: S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Start new conversation button's tooltip not updating in DM narrow when direct messages are disabled.
5 participants