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

onboarding: Add one-time go-to-conversation tooltip. #32169

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

onboarding: Add one-time go-to-conversation tooltip. #32169

wants to merge 1 commit into from

Conversation

userAdityaa
Copy link
Collaborator

@userAdityaa userAdityaa commented Oct 29, 2024 •
edited
Loading

This commit helps for configuring one-time tippy tooltip when the go to conversation button is active, as previously it was difficult for user to find out about the button.

Fixes: #32166

Before (No indication)

Screen.Recording.2024-10-30.at.4.43.02.AM.mov

After (Fire up Tooltip)

Screen.Recording.2024-12-17.at.3.27.34.AM.mov

Screenshot (Tooltip)

Screenshot 2024-12-17 at 1 14 52 PM
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: popovers Popovers, including general message actions. priority: high labels Oct 29, 2024
@userAdityaa userAdityaa changed the title tippy_tooltip: Configured the tippy tooltip to fire one-time tippy_tooltip: Configured the tippy tooltip to fire one-time. Oct 29, 2024
@userAdityaa
Copy link
Collaborator Author

Hey @alya, Ready for review!

@userAdityaa userAdityaa changed the title tippy_tooltip: Configured the tippy tooltip to fire one-time. tooltips: Configure tippy tooltips to fire only once. Nov 12, 2024
lincolncpp
lincolncpp reviewed Nov 13, 2024
View reviewed changes
web/src/compose_recipient.ts Outdated
Comment on lines 99 to 105
const instance = tippy(arrow_element, {
content: "Go to conversation",
trigger: "manual",
placement: "top",
});
Copy link

Choose a reason for hiding this comment

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

Instead of creating a new tippy instance every time show_tippy_tooltip is called, you could check if an instance already exists on the element and reuse it. This avoids creating duplicate tooltips and improves performance.

Suggested change
const instance = tippy(arrow_element, {
content: "Go to conversation",
trigger: "manual",
placement: "top",
});
const instance = arrow_element._tippy || tippy(arrow_element, {
content: "Go to conversation",
trigger: "manual",
placement: "top",
});

@prakhar1144
Copy link
Member

@userAdityaa the issue mentions:

we should add one-time tooltips (one for topics, one for DMs) highlighting this button.

Here, by "one-time" it means -- "visible to a user only once". Currently it will appear every time a user composes a message.

Also, I don't think the PR aligns with this point mentioned in the issue:

You haven't clicked the button (which is the mechanism that would dismiss it).

Sidenote: Fix the commit message as per commit guidelines.

@zulipbot zulipbot added size: M and removed size: S labels Dec 16, 2024
@userAdityaa
Copy link
Collaborator Author

userAdityaa commented Dec 16, 2024 •
edited
Loading

Hey @alya, Ready for review!
All the comments have been looked into and fixed.
Uploading the proper fix video in some hour.

@alya
Copy link
Contributor

alya commented Dec 16, 2024

As requested in the PR template, please post screenshots of all your changes in the PR description. It is very difficult to review a video.

@userAdityaa
Copy link
Collaborator Author

Hey @alya, Done with that. After this changes now the user will see the fire-tooltip only once and it will only get close by clicking on the arrow key.

@alya
Copy link
Contributor

alya commented Dec 17, 2024

OK, but I still don't see a screenshot of the tooltip.

timabbott
timabbott reviewed Dec 17, 2024
View reviewed changes
web/src/compose_recipient.ts Outdated
});

instance.show();
localStorage.setItem("conversation_tooltip", "true");
Copy link
Member

Choose a reason for hiding this comment

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

Don't use browser local storage, see git grep OnboardingStep for how to implement a one-time notice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Im fixing it right now.

@timabbott timabbott changed the title tooltips: Configure tippy tooltips to fire only once. onboarding: Add one-time go-to-conversation tooltip. Dec 17, 2024
@timabbott
Copy link
Member

I fixed the PR title to be appropriately descriptive, and this needs to be reworked to store the data correctly, but that probably is independent of the UI.

@saubhagya-patel
Copy link
Collaborator

Hi @userAdityaa and @timabbott, I have resolved the issue locally by implementing it as an onboarding step. Although I was initially unable to fix the failing tests yesterday, I'm happy to report that the tests have now been successfully fixed.

Would it be okay for me to open a PR? Thank you.

@userAdityaa userAdityaa force-pushed the branch-32166 branch 2 times, most recently from ab7be76 to 30417fd Compare December 18, 2024 17:24
@zulipbot zulipbot added size: L and removed size: M labels Dec 18, 2024
@zulipbot zulipbot added size: M and removed size: L labels Dec 18, 2024
@userAdityaa
tooltips: Configure tippy tooltips to fire only once.
a0fd631 
This commit helps for configuring one-time tippy tooltip when the go to
conversation button is active, as previously it was difficult for user to
find out about the button.

Fixes zulip#32166
@alya
Copy link
Contributor

alya commented Dec 19, 2024

@saubhagya-patel , sure, that's fine. Please be sure to follow https://zulip.readthedocs.io/en/latest/contributing/continuing-unfinished-work.html and tag @userAdityaa for a review.

@saubhagya-patel saubhagya-patel mentioned this pull request Dec 19, 2024
Open
12 tasks
@saubhagya-patel
Copy link
Collaborator

@alya, I have opened a PR #32773 addressing the issue, following this comment.

Following the continuing unfinished work documentation, please note that PR #32773 takes a different approach than the one in this PR. Specifically, this PR adds the tooltip when updating the class of the "go-to conversation" button, whereas PR #32773 introduces the tooltip when the #compose-textarea is focused.

Thank you.

@userAdityaa
Copy link
Collaborator Author

Hey @saubhagya-patel, If you don't mind can you review this PR. And if there is something wrong you can just inform that to me. Thank you in advance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lincolncpp lincolncpp lincolncpp left review comments

@timabbott timabbott timabbott left review comments

Assignees
No one assigned
Labels
area: popovers Popovers, including general message actions. priority: high size: M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add one-time intro tooltip for "go to conversation" button
7 participants
@userAdityaa @prakhar1144 @alya @timabbott @saubhagya-patel @lincolncpp @zulipbot