Skip to content

Maryia/DTRA-453/fix: Tooltips are not working properly in mobile, require holding i button #11027

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
Merged
maryia-deriv merged 7 commits into from
Nov 2, 2023
Merged

Maryia/DTRA-453/fix: Tooltips are not working properly in mobile, require holding i button #11027

maryia-deriv merged 7 commits into from
Nov 2, 2023

Conversation

@maryia-deriv
Copy link
Contributor

@maryia-deriv maryia-deriv commented Oct 29, 2023

Changes:

  • Fix information tooltips in responsive that currently require holding i button. Information tooltips should remain open after releasing i button after clicking it.
  • Ensure that all icons in mobile header of 'Trader's hub' open links (if they have any) upon the 1st click on all mobile devices including iOS because this task also includes partial reversion of changes from chore:Fix tap twice for manage account #9562

Screenshots:

Please provide some screenshots of the change.

@vercel
Copy link

vercel bot commented Oct 29, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
deriv-app ✅ Ready (Inspect) Visit Preview Nov 2, 2023 10:43am

maryia-deriv
maryia-deriv commented Oct 29, 2023
View reviewed changes
packages/components/src/hooks/use-hover.ts
}
};
}
return undefined;
Copy link
Contributor Author

@maryia-deriv maryia-deriv Oct 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

reversion of changes from #9562 that caused all popovers (tooltips) in DTrader to require a long tap instead of a single tap.

Copy link
Contributor

@balakrishna-deriv balakrishna-deriv Oct 30, 2023

Choose a reason for hiding this comment

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

This is fine.
@suisin-deriv Please take note. The touch events handler should be on its own hook. It should not be related to the use-hover hook.

maryia-deriv
maryia-deriv commented Oct 29, 2023
View reviewed changes
packages/core/src/App/Containers/Layout/header/default-mobile-links.tsx
<Popover
alignment='bottom'
classNameBubble='account-settings-toggle__tooltip'
message={<Localize i18n_default_text='Manage account settings' />}
Copy link
Contributor Author

@maryia-deriv maryia-deriv Oct 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This DefaultMobileLinks component is rendered for mobile only, and we don't need this popover tooltip for binary link to Account Settings in mobile since there is no hover in mobile, and it should redirect immediately as per requirements.
*Note: Even with this popover, it was always working for Android but not for iOS. Hence, removing to have consistent behavior across devices.

maryia-deriv
maryia-deriv commented Oct 29, 2023
View reviewed changes
packages/core/src/App/Containers/Layout/header/traders-hub-onboarding.tsx
classNameBubble='account-settings-toggle__tooltip'
alignment='bottom'
message={<Localize i18n_default_text='View onboarding' />}
message={!is_mobile && <Localize i18n_default_text='View onboarding' />}
Copy link
Contributor Author

@maryia-deriv maryia-deriv Oct 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

In mobile, we don't need a popover tooltip for this icon that redirects to Onboarding since there is no hover in mobile, and it should redirect immediately as per requirements.
*Note: Even with this popover, it was always working for Android but not for iOS. Hence, removing to have consistent behavior across devices.

@coveralls
Copy link

coveralls commented Oct 29, 2023 •
edited
Loading

Coverage Status

coverage: 26.45% (+0.009%) from 26.441% when pulling c0043c3 on maryia-deriv:maryia/dtra-453-2/popovers-fix into e6f5568 on binary-com:master.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 29, 2023 •
edited
Loading

A production App ID was automatically generated for this PR. (log)

Click here to copy & paste above information. 
- **PR**: [https://github.com/binary-com/deriv-app/pull/11027](https://github.com/binary-com/deriv-app/pull/11027)
- **URLs**:
    - **w/ App ID + Server**: https://deriv-app-git-fork-maryia-deriv-maryia-dtra-453-2popovers-fix.binary.sx?qa_server=red.derivws.com&app_id=30366
    - **Original**: https://deriv-app-git-fork-maryia-deriv-maryia-dtra-453-2popovers-fix.binary.sx
- **App ID**: `30366`

@github-actions
Copy link
Contributor

github-actions bot commented Oct 29, 2023 •
edited
Loading

🚨 Lighthouse report for the changes in this PR:

Category Score
🔺 Performance 15
🟧 Accessibility 75
🟢 Best practices 92
🟧 SEO 85
🟧 PWA 80

Lighthouse ran with https://deriv-app-git-fork-maryia-deriv-maryia-dtra-453-2popovers-fix.binary.sx/

@maryia-deriv maryia-deriv force-pushed the maryia/dtra-453-2/popovers-fix branch from 8d64e01 to 7249eb9 Compare October 29, 2023 17:11
@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@maryia-deriv maryia-deriv merged commit c501f78 into deriv-com:master Nov 2, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2023

🚀 Smoke test run (2) passed successfully!

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

Reviewers

@balakrishna-deriv balakrishna-deriv balakrishna-deriv approved these changes

@akmal-deriv akmal-deriv akmal-deriv approved these changes

@ali-hosseini-deriv ali-hosseini-deriv Awaiting requested review from ali-hosseini-deriv

@matin-deriv matin-deriv Awaiting requested review from matin-deriv matin-deriv is a code owner

+2 more reviewers

@henry-deriv henry-deriv henry-deriv approved these changes

@kate-deriv kate-deriv kate-deriv approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Components Core

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@maryia-deriv @coveralls @balakrishna-deriv @akmal-deriv @henry-deriv @kate-deriv