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): DLT-1757 round button alignment #314

Merged
merged 27 commits into from
May 16, 2024
Merged

Conversation

braddialpad
Copy link
Contributor

@braddialpad braddialpad commented May 15, 2024

fix(tooltip): round button alignment

Obligatory GIF (super important!)

Obligatory GIF

🛠️ Type Of Change

These types will increment the version number on release:

  • Fix

📖 Jira Ticket

📖 Description

  • Changed tooltip arrows from custom CSS to the built in Tippy arrow SVG, which it automatically aligns correctly
  • No longer rendering tooltips with a custom tippy render function. (Why were we doing this in the first place?)
  • Still using the old style custom render function for popover, because that one will be a ton of work to fix.
  • When rendering this way, the default offset had to change. I will have to check any custom offsets set in product because they may be incorrect after this change.
  • Remove aria attrs because they are handled by tippy
  • Remove lazy show because that's handled by tippy

💡 Context

Noticed this issue in my prior PR #311

The tooltip arrow was not rendering correctly on circle buttons because we were using custom css from who knows how long ago, and it was rendering the arrow in a fixed position. Tippy renders it's arrow dynamically to be in the center so I changed it to that instead. In order to do this I had to get rid of our custom override of the tippy render function, which seemed to have no real purpose, and it disabled a ton of default tippy functionality and options that we could have been taking advantage of.

There are a ton of other things we can improve in this component and the popover one now, but I'm trying to keep the scope of this somewhat reasonable.

BEFORE

Screenshot 2024-05-15 at 1 40 03 PM

AFTER

Screenshot 2024-05-15 at 1 37 36 PM

📝 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 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. Read docs here: Dialtone Vue Sync Script

For all CSS changes:

  • I have used design tokens whenever possible.
  • I have considered how this change will behave on different screen sizes.
  • I have visually validated my change in light and dark mode.

🔮 Next Steps

check usage of offset in product.

📷 Screenshots / GIFs

See Context

🔗 Sources

https://atomiks.github.io/tippyjs/v6/all-props

@braddialpad braddialpad added the visual-test-ready Add this tag when the PR is ready for visual test, to trigger GHA visual tests label May 15, 2024
@braddialpad braddialpad self-assigned this May 15, 2024
@braddialpad
Copy link
Contributor Author

@francisrupert the shape of the arrow changed slightly since it's now just using the built in tippy arrow. Should we make a custom SVG to make it look like the previously used arrow? For reference this is the svg for the default tippy arrow.

<svg width="16" height="6" xmlns="http://www.w3.org/2000/svg"><path d="M0 6s1.796-.013 4.67-3.615C5.851.9 6.93.006 8 0c1.07-.006 2.148.887 3.343 2.385C14.233 6.005 16 6 16 6H0z"></svg>

@braddialpad braddialpad changed the title fix(tooltip): round button alignment fix(tooltip): DLT-1757 round button alignment May 15, 2024
Copy link

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

Copy link

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

Copy link

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

Comment on lines 519 to 534
@import 'tippy.js/dist/svg-arrow.css';

.tippy-box[data-popper-reference-hidden] {
.d-tooltip {
visibility: hidden;
pointer-events: none;
}
}

.tippy-box > .tippy-svg-arrow {
fill: var(--dt-color-surface-contrast);
}

.tippy-box[data-animation='fade'][data-state='hidden'] {
opacity: 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we move this to dialpad css?, since this is not an scoped style, we will be importing @import 'tippy.js/dist/svg-arrow.css'; globally when this component is being initialized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is tippy.js is not a dependency of dialtone-css, so it won't be able to import it. Scoping it also won't work because the tooltip renders outside of the component at the document body. I think it's probably okay just to leave global 🤷

@iropolo
Copy link
Contributor

iropolo commented May 16, 2024

Seems like there is some issues in the test.

it('should set default classes', () => {
  expect(tooltip.classes('d-tooltip__arrow-tippy--top')).toBe(true);
});

This one seems like it is not necessary anymore since we are not handling the classes anymore.

Then there is some issues related to show prop or isShown watcher.

Copy link

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

1 similar comment
Copy link

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

Copy link

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

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.

Cool and useful improvement, thanks.

Copy link

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

Copy link

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

Copy link

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

Copy link

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

@braddialpad
Copy link
Contributor Author

Fixed svg shape, merging.

@braddialpad braddialpad merged commit eabd110 into staging May 16, 2024
11 checks passed
@braddialpad braddialpad deleted the fix/tooltip-align branch May 16, 2024 22:26
juliodialpad pushed a commit that referenced this pull request May 16, 2024
# [9.36.0](dialtone/v9.35.0...dialtone/v9.36.0) (2024-05-16)

### Bug Fixes

* **Tooltip:** DLT-1757 round button alignment ([#314](#314)) ([eabd110](eabd110))

### Features

* DLT-1756 add unread mention count ([#312](#312)) ([ff21d01](ff21d01))
Copy link

🎉 This PR is included in version 9.36.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

juliodialpad pushed a commit that referenced this pull request May 16, 2024
## [8.31.2](dialtone-css/v8.31.1...dialtone-css/v8.31.2) (2024-05-16)

### Bug Fixes

* **Tooltip:** DLT-1757 round button alignment ([#314](#314)) ([eabd110](eabd110))
Copy link

🎉 This PR is included in version 8.31.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

juliodialpad pushed a commit that referenced this pull request May 16, 2024
# [2.132.0](dialtone-vue2/v2.131.0...dialtone-vue2/v2.132.0) (2024-05-16)

### Bug Fixes

* **Tooltip:** DLT-1757 round button alignment ([#314](#314)) ([eabd110](eabd110))

### Features

* DLT-1756 add unread mention count ([#312](#312)) ([ff21d01](ff21d01))
Copy link

🎉 This PR is included in version 2.132.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

juliodialpad pushed a commit that referenced this pull request May 16, 2024
# [3.125.0](dialtone-vue3/v3.124.0...dialtone-vue3/v3.125.0) (2024-05-16)

### Bug Fixes

* **Tooltip:** DLT-1757 round button alignment ([#314](#314)) ([eabd110](eabd110))

### Features

* DLT-1756 add unread mention count ([#312](#312)) ([ff21d01](ff21d01))
Copy link

🎉 This PR is included in version 3.125.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released visual-test-ready Add this tag when the PR is ready for visual test, to trigger GHA visual tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants