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

refactor(popover, hovercard): DLT-2245 separate Hovercard from Popover #606

Merged
merged 4 commits into from
Jan 23, 2025
Merged

refactor(popover, hovercard): DLT-2245 separate Hovercard from Popover #606

merged 4 commits into from
Jan 23, 2025

Conversation

ninamarina
Copy link
Contributor

@ninamarina ninamarina commented Jan 14, 2025 •
edited
Loading

Separate Hovercard from Popover

Obligatory GIF (super important!)

Obligatory GIF

🛠️ Type Of Change

These types will increment the version number on release:

  • Fix
  • Feature
  • Performance Improvement
  • Refactor

📖 Jira Ticket

DLT-2245

📖 Description

Removes the Hovercard logic from Popover to Hovercard. Now the hover mouse events are controlled by the Hovercard and state is in sync via the open prop of the Popover.
The objective is to simplify a bit the Popover code and to decouple the Hovercard logic from it.
The behavior should remain the same.

💡 Context

The idea to do this came from this PR: #597

📝 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. Read docs here: Dialtone Vue Sync Script
  • I have validated components with a screen reader.
  • I have validated components keyboard navigation.

@ninamarina ninamarina force-pushed the DLT-2245/refactor-hovercard branch from 6289190 to 94c58e4 Compare January 14, 2025 18:52
@ninamarina ninamarina force-pushed the DLT-2245/refactor-hovercard branch from 94c58e4 to 8cf047e Compare January 14, 2025 19:03
@github-actions GitHub Actions
Copy link

Please add either the visual-test-ready or no-visual-test label to this PR depending on whether you want to run visual tests or not.
It is recommended to run visual tests if your PR changes any UI. ‼️

@ninamarina ninamarina added the no-visual-test Add this tag when the PR does not need visual testing label Jan 14, 2025
@ninamarina ninamarina changed the title refactor(popover, hovercard): DLT-2245 separate hovercard from popover refactor(popover, hovercard): DLT-2245 separate Hovercard from Popover Jan 14, 2025
@ninamarina ninamarina marked this pull request as ready for review January 14, 2025 19:42
@braddialpad
Copy link
Contributor

I'm going to run visual tests here just to make sure

@braddialpad braddialpad added visual-test-ready Add this tag when the PR is ready for visual test, to trigger GHA visual tests and removed no-visual-test Add this tag when the PR does not need visual testing labels Jan 16, 2025
@braddialpad
Copy link
Contributor

Couple errors, but don't seem to be anything to do with your change.

braddialpad
braddialpad reviewed Jan 17, 2025
View reviewed changes
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 pretty good IMO, just one question.

packages/dialtone-vue2/components/popover/popover.vue
this.isOpen = true;
}, this.enterDelay);
onMouseEnter () {
this.$emit('mouseenter-popover');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the event be called "mouseenter-popover-anchor" since there could also be the case of the mouse entering the popover dialog?

@ninamarina ninamarina force-pushed the DLT-2245/refactor-hovercard branch from 92beb87 to d6ae060 Compare January 17, 2025 18:56
@github-actions GitHub Actions
Copy link

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

juliodialpad
juliodialpad approved these changes Jan 22, 2025
View reviewed changes
Copy link
Collaborator

@juliodialpad juliodialpad left a comment

Choose a reason for hiding this comment

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

Working perfectly, nice clean up, thanks!

braddialpad
braddialpad approved these changes Jan 22, 2025
View reviewed changes
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.

Sorry I missed re-reviewing this. Looks good!

@ninamarina ninamarina merged commit d11c5e4 into staging Jan 23, 2025
11 checks passed
@ninamarina ninamarina deleted the DLT-2245/refactor-hovercard branch January 23, 2025 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@juliodialpad juliodialpad juliodialpad approved these changes

@braddialpad braddialpad braddialpad approved these changes

@iropolo iropolo Awaiting requested review from iropolo iropolo is a code owner

@francisrupert francisrupert Awaiting requested review from francisrupert francisrupert is a code owner

Assignees
No one assigned
Labels
visual-test-ready Add this tag when the PR is ready for visual test, to trigger GHA visual tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ninamarina @braddialpad @juliodialpad