Skip to content

Add Tooltips to all non-text actions/buttons in New Expensify #4499

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

Closed
mananjadhav opened this issue Aug 7, 2021 · 71 comments
Closed

Add Tooltips to all non-text actions/buttons in New Expensify #4499

mananjadhav opened this issue Aug 7, 2021 · 71 comments
Assignees
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Task

Comments

@mananjadhav
Copy link
Collaborator

mananjadhav commented Aug 7, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Action Performed:

  1. Click on '+' icon on Chat
  2. Click on Add attachment and select an image
  3. AttachmentModal opens and has an action for download but no text

This has to be handled at multiple places such as Workspace Image Picker, Profile Picture Image Picker, etc.

Expected Result:

This is how Whatsapp shows
image

Actual Result:

Shows no tooltip/text right now. Only icons.

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number:
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Upwork job link: https://www.upwork.com/ab/applicants/1426188061929398272/job-details

View all open jobs on Upwork

@mananjadhav mananjadhav added AutoAssignerTriage Auto assign issues for triage to an available triage team member Daily KSv2 labels Aug 7, 2021
@MelvinBot
Copy link

Triggered auto assignment to @zanyrenney (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Aug 7, 2021
@mananjadhav
Copy link
Collaborator Author

Proposal

Enclose all the action items with <Tooltip text={text}> component, which will be picked from translation.

For example, download button would require changes

In HeaderWithCloseButton, downloadButton block would look like:


props.shouldShowDownloadButton && (
	<Tooltip text={props.translate('common.download')}>
	    <TouchableOpacity
	        onPress={props.onDownloadButtonPress}
	        style={[styles.touchableButtonImage]}
	    >
	        <Icon src={Download} />
	    </TouchableOpacity>
    </Tooltip>
)

and translations would be as followed:

In en: {
	common: {
		download: 'Download'
	}
}

and es: {
	common: {
		download: 'Descargar'
	}
} 

@MelvinBot
Copy link

Triggered auto assignment to @ctkochan22 (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@zanyrenney zanyrenney assigned ctkochan22 and unassigned ctkochan22 and zanyrenney Aug 9, 2021
@zanyrenney
Copy link
Contributor

sorry @ctkochan22 accidentally unassigned you so reassigned

@ctkochan22 ctkochan22 added Task Weekly KSv2 External Added to denote the issue can be worked on by a contributor and removed Daily KSv2 labels Aug 9, 2021
@ctkochan22 ctkochan22 removed their assignment Aug 10, 2021
@MelvinBot
Copy link

Triggered auto assignment to @michaelhaxhiu (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@MelvinBot
Copy link

Triggered auto assignment to @michelle-thompson (Design), see these Stack Overflow questions for more details.

@michaelhaxhiu
Copy link
Contributor

Sorry on the delay! Let's get quick input from Design, since this will be a UI theme that's widespread in New Expensify.

@MelvinBot MelvinBot removed the Overdue label Aug 13, 2021
@shawnborton
Copy link
Contributor

Makes sense to me.

@michaelhaxhiu
Copy link
Contributor

michaelhaxhiu commented Aug 13, 2021

Do we want to use the same UI theme as WhatsApp? Or do we want a specific spacing, font, and tooltip box?

@mananjadhav
Copy link
Collaborator Author

mananjadhav commented Aug 25, 2021

@parasharrajat I've fixed the broken pointer.

Is this desired?
Screenshot 2021-08-25 04:14:12

I also checked the AvatarWithImagePicker tooltip. I guess you're right it isn't needed. Looks a bit weird with the avatar picture on. What are your thoughts? @shawnborton @iwiznia @michaelhaxhiu

@michaelhaxhiu
Copy link
Contributor

Thanks for attempting to fix the placement of the floating tooltips that @parasharrajat brought up!

I am having a hard time seeing the how the tooltip placement changed in the Profile picture example. Did it move it up a few pixels?

@mananjadhav
Copy link
Collaborator Author

No, it isn't changed from my previous commit. But what is happening is that tooltip is showing on the AvatarImage, which looks a bit off when there's a profile picture added.

@parasharrajat
Copy link
Member

parasharrajat commented Aug 25, 2021

I meant to point out that Edit Profile tooltip should be over or below the pencil icon instead of profile avatar.

Another issue which I faced was that after tooltip is visible and you click edit ✏️, tooltip never goes away.

@mananjadhav
Copy link
Collaborator Author

@parasharrajat That is what I had tried earlier. The pencil icon is absolute positioned and with the existing tooltip behavior, I haven't been able to position the tooltip properly on the icon.

@michaelhaxhiu
Copy link
Contributor

michaelhaxhiu commented Aug 25, 2021

I meant to point out that Edit Profile tooltip should be over or below the pencil icon instead of profile avatar.

Now I follow you 👌. The tooltip for the profile picture should probably show under the pencil. Right @shawnborton?

@shawnborton
Copy link
Contributor

Can it launch directly above the gray circle that has the pencil icon?

@mananjadhav
Copy link
Collaborator Author

At the moment, our Tooltip doesn't work well with absolute positioned items. Considering the effort involved, we didn't include that in the current scope.

Hence, we skipped the FAB too.
#4499 (comment)

@shawnborton
Copy link
Contributor

Then I would just skip this one yeah?

@mananjadhav
Copy link
Collaborator Author

Then I would just skip this one yeah?

Can do. @michaelhaxhiu @iwiznia What do you think?

@iwiznia
Copy link
Contributor

iwiznia commented Aug 25, 2021

I am fine skipping for now

@michaelhaxhiu
Copy link
Contributor

michaelhaxhiu commented Aug 25, 2021

Agree, let's skip showing this tooltip for now. We can revisit this later. Thanks for bringing it up @parasharrajat

@parasharrajat
Copy link
Member

Regression. Icons the moving up in the composer on Web.

image

@mananjadhav
Copy link
Collaborator Author

mananjadhav commented Aug 26, 2021

PR Raised

@botify
Copy link

botify commented Aug 30, 2021

🚀 Deployed to staging by @iwiznia in version: 1.0.88-3 🚀

platform result
🤖 android 🤖 cancelled 🔪
🖥 desktop 🖥 cancelled 🔪
🍎 iOS 🍎 cancelled 🔪
🕸 web 🕸 success ✅

@botify
Copy link

botify commented Sep 1, 2021

🚀 Deployed to production by @roryabraham in version: 1.0.90-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

@michaelhaxhiu
Copy link
Contributor

@roryabraham quick bud check -- this botify chart makes me believe that iOS isn't ready for deploy, which means we shouldn't start the 7 day payment countdown? Is that accurate?

I'd like to make sure I understand for future cases.

@MelvinBot MelvinBot removed the Overdue label Sep 6, 2021
@mananjadhav
Copy link
Collaborator Author

@michaelhaxhiu @roryabraham fyi, this PR primarily has an impact on Desktop and Web.

@michaelhaxhiu
Copy link
Contributor

michaelhaxhiu commented Sep 7, 2021

Yep understood as this relates to tooltips that appear upon mouse hover over - that's why I'm a little bit unsure how to handle the botify error concerning iOS (i.e. does it matter)?

@roryabraham
Copy link
Contributor

Sorry for the delayed reply and red-herring comment. That failed iOS prod deploy happened completely independently of this issue, and was completed manually after-the-fact. So I think that this has been solved on prod for more than a week at this point and can be paid/closed out. Thanks!

@michaelhaxhiu
Copy link
Contributor

Paid!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Task
Projects
None yet
Development

No branches or pull requests