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

fix(tooltip): stop showing tootip for touch based devices #131

Merged
merged 11 commits into from
Feb 7, 2024

Conversation

kdsanskar
Copy link
Contributor

@kdsanskar kdsanskar commented Jan 29, 2024

fix(tooltip): stop showing tootip for touch based devices

Obligatory GIF (super important!)

Obligatory GIF

🛠️ Type Of Change

These types will increment the version number on release:

  • Fix
  • Feature
  • Performance Improvement
  • Refactor

These types will not increment the version number, but will still deploy to documentation site on release:

  • Documentation
  • Build system
  • CI
  • Style (code style changes, not css changes)
  • Test
  • Other (chore)

📖 Jira Ticket

https://dialpad.atlassian.net/browse/DLT-1271

📖 Description

Hide the tooltip for touch based device.

💡 Context

Tooltip should not showup for touch based devices as there is no hover event, yet it is showing up when user clicks the anchor element which is not even hover.

📝 Checklist

For all PRs:

  • I have ensured no private Dialpad links or info are in the code or pull request description (Dialtone is a public repo!).
  • I have reviewed my changes.
  • I have added all relevant documentation.
  • I have considered the performance impact of my change.

For all Vue changes:

  • I have added / updated unit tests.
  • I have made my changes in Vue 2 and Vue 3. Note: you may sync your changes from Vue 2 to Vue 3 (or vice versa) using the ./scripts/dialtone-vue-sync.sh script.
  • I have validated components with a screen reader.
  • I have validated components keyboard navigation.

🔮 Next Steps

📷 Screenshots / GIFs

🔗 Sources

Copy link

✔️ Deploy previews ready!
😎 Dialtone preview: https://dialtone.dialpad.com/deploy-previews/pr-131/
😎 Dialtone-vue 2 preview: https://dialtone.dialpad.com/vue/deploy-previews/pr-131/
😎 Dialtone-vue 3 the preview: https://dialtone.dialpad.com/vue3/deploy-previews/pr-131/

@kdsanskar kdsanskar changed the title DLT-1271: Stop trigger of hover event when clicked on touch based environment DLT-1271: Stop showing tootip for touch based devices Jan 31, 2024
Copy link

✔️ Deploy previews ready!
😎 Dialtone preview: https://dialtone.dialpad.com/deploy-previews/pr-131/
😎 Dialtone-vue 2 preview: https://dialtone.dialpad.com/vue/deploy-previews/pr-131/
😎 Dialtone-vue 3 the preview: https://dialtone.dialpad.com/vue3/deploy-previews/pr-131/

@kdsanskar kdsanskar changed the title DLT-1271: Stop showing tootip for touch based devices fix(tooltip): stop showing tootip for touch based devices Jan 31, 2024
@kdsanskar kdsanskar added the no-visual-test Add this tag when the PR does not need visual testing label Jan 31, 2024
Copy link

✔️ Deploy previews ready!
😎 Dialtone preview: https://dialtone.dialpad.com/deploy-previews/pr-131/
😎 Dialtone-vue 2 preview: https://dialtone.dialpad.com/vue/deploy-previews/pr-131/
😎 Dialtone-vue 3 the preview: https://dialtone.dialpad.com/vue3/deploy-previews/pr-131/

Copy link

✔️ Deploy previews ready!
😎 Dialtone preview: https://dialtone.dialpad.com/deploy-previews/pr-131/
😎 Dialtone-vue 2 preview: https://dialtone.dialpad.com/vue/deploy-previews/pr-131/
😎 Dialtone-vue 3 the preview: https://dialtone.dialpad.com/vue3/deploy-previews/pr-131/

Copy link
Contributor

@braddialpad braddialpad left a comment

Choose a reason for hiding this comment

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

Looks good thanks!

};
},

computed: {
// whether the tooltip is visible or not.
isVisible () {
return this.isShown && this.enabled && (!!this.message.trim() || !!this.$slots.default);
return this.isShown && this.enabled && (!!this.message.trim() || !!this.$slots.default) && !this.isTouchDevice;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to improve this computed property, could be something like this?

isVisible() {
  const hasMessage = !!this.message.trim();
  const hasDefaultSlot = !!this.$slots.default;
  const isDeviceCompatible = !this.isTouchDevice; 

  const shouldBeVisible = this.isShown && this.enabled && (hasMessage || hasDefaultSlot);

  return shouldBeVisible && isDeviceCompatible;
},

I guess it will work as this way, double check it please to be sure, and make this update also in the vue3 version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

@iropolo iropolo left a comment

Choose a reason for hiding this comment

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

Looks good, thanks

@ninamarina
Copy link
Contributor

ninamarina commented Feb 1, 2024

The code looks good, though it seems it still shows the tooltip every once in a while when tapping the anchor.
I tested it on my phone, every ten taps the tooltip shows:
image

It's still much better that what we have now, where the tooltip is shown in every tap.

Copy link

github-actions bot commented Feb 6, 2024

✔️ Deploy previews ready!
😎 Dialtone preview: https://dialtone.dialpad.com/deploy-previews/pr-131/
😎 Dialtone-vue 2 preview: https://dialtone.dialpad.com/vue/deploy-previews/pr-131/
😎 Dialtone-vue 3 the preview: https://dialtone.dialpad.com/vue3/deploy-previews/pr-131/

@kdsanskar
Copy link
Contributor Author

kdsanskar commented Feb 6, 2024

The code looks good, though it seems it still shows the tooltip every once in a while when tapping the anchor. I tested it on my phone, every ten taps the tooltip shows:

image

@ninamarina This issue is cuz mouseenter event is also getting triggered after touchstart event, I have tried to stop that using relatedTarget property but still even after that in chrome browser once in while what is happening is that property relatedTarget is getting not null for touch event which it should not, meanwhile I have tested in firefox it worked fine.

@ninamarina @braddialpad Please let know if there is something more we can do fix it, otherwise it looks like a browser issue in chrome.

refer: https://copyprogramming.com/howto/jquery-on-click-triggers-mouseover-on-touch-device

Copy link

github-actions bot commented Feb 7, 2024

✔️ Deploy previews ready!
😎 Dialtone preview: https://dialtone.dialpad.com/deploy-previews/pr-131/
😎 Dialtone-vue 2 preview: https://dialtone.dialpad.com/vue/deploy-previews/pr-131/
😎 Dialtone-vue 3 the preview: https://dialtone.dialpad.com/vue3/deploy-previews/pr-131/

Copy link

github-actions bot commented Feb 7, 2024

✔️ Deploy previews ready!
😎 Dialtone preview: https://dialtone.dialpad.com/deploy-previews/pr-131/
😎 Dialtone-vue 2 preview: https://dialtone.dialpad.com/vue/deploy-previews/pr-131/
😎 Dialtone-vue 3 the preview: https://dialtone.dialpad.com/vue3/deploy-previews/pr-131/

Copy link

github-actions bot commented Feb 7, 2024

✔️ Deploy previews ready!
😎 Dialtone preview: https://dialtone.dialpad.com/deploy-previews/pr-131/
😎 Dialtone-vue 2 preview: https://dialtone.dialpad.com/vue/deploy-previews/pr-131/
😎 Dialtone-vue 3 the preview: https://dialtone.dialpad.com/vue3/deploy-previews/pr-131/

Copy link

github-actions bot commented Feb 7, 2024

✔️ Deploy previews ready!
😎 Dialtone preview: https://dialtone.dialpad.com/deploy-previews/pr-131/
😎 Dialtone-vue 2 preview: https://dialtone.dialpad.com/vue/deploy-previews/pr-131/
😎 Dialtone-vue 3 the preview: https://dialtone.dialpad.com/vue3/deploy-previews/pr-131/

@kdsanskar kdsanskar merged commit 0c1be19 into staging Feb 7, 2024
8 checks passed
@kdsanskar kdsanskar deleted the dlt-1221-stop-showing-tooltip-for-touch-device branch February 7, 2024 17:35
braddialpad pushed a commit that referenced this pull request Feb 7, 2024
## [3.107.2](dialtone-vue3/v3.107.1...dialtone-vue3/v3.107.2) (2024-02-07)

### Bug Fixes

* **Tooltip:** stop showing tootip for touch based devices ([#131](#131)) ([0c1be19](0c1be19))
braddialpad pushed a commit that referenced this pull request Feb 7, 2024
## [2.114.2](dialtone-vue2/v2.114.1...dialtone-vue2/v2.114.2) (2024-02-07)

### Bug Fixes

* **Tooltip:** stop showing tootip for touch based devices ([#131](#131)) ([0c1be19](0c1be19))
braddialpad pushed a commit that referenced this pull request Feb 7, 2024
## [9.11.6](dialtone/v9.11.5...dialtone/v9.11.6) (2024-02-07)

### Bug Fixes

* **Tooltip:** stop showing tootip for touch based devices ([#131](#131)) ([0c1be19](0c1be19))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-visual-test Add this tag when the PR does not need visual testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants