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

feat(hovercard): DP-119841 hovercard fixes for Fast Connect #597

Merged
merged 7 commits into from
Dec 18, 2024

Conversation

ninamarina
Copy link
Contributor

@ninamarina ninamarina commented Dec 16, 2024

Hovercard fixes for Fast Connect

πŸ› οΈ Type Of Change

These types will increment the version number on release:

  • Fix
  • Feature
  • Performance Improvement
  • Refactor

πŸ“– Jira Ticket

DP-119841

πŸ“– Description

Some features needed for Fast Connect, and a bug fix.

  • Adds "open" prop to Hovercard, to allow to programmatically open / close the hovercard, works like the "open" prop for the Popover component

  • Fixes a bug that happened when hovering mentions that are close.
    To fix this I removed the special behavior for the timer, that when changing from one hovercard to another would reduce the delay. I think it doesn't have a noticeable impact for the user and it was causing the hovercard to change to another one when passing through a mention, making it imposible to access the buttons for this case:

Before Now
mentions-prev mentions
  • Adds enterDelay and leaveDelay params

πŸ’‘ Context

This is for the Launchathon of Fast Connect: https://www.figma.com/design/gOHiwlDi8bkaV2rkDeR0NP/Fast-Connect?node-id=4921-60460&p=f&t=8uDpFPXJuc88aZzI-0

@ninamarina ninamarina force-pushed the fix/hovercard-fastconnect branch from 29adfc9 to cf13990 Compare December 16, 2024 19:19
@ninamarina ninamarina changed the title add open prop to hovercard feat(hovercard): DP-119841 hovercard fixes for Fast Conenct Dec 16, 2024
@ninamarina ninamarina changed the title feat(hovercard): DP-119841 hovercard fixes for Fast Conenct feat(hovercard): DP-119841 hovercard fixes for Fast Connect Dec 16, 2024
@ninamarina ninamarina force-pushed the fix/hovercard-fastconnect branch from cf13990 to 29d3f4c Compare December 16, 2024 20:23
@ninamarina ninamarina force-pushed the fix/hovercard-fastconnect branch from 73f9a04 to 4d63dd2 Compare December 16, 2024 20:56
@ninamarina ninamarina force-pushed the fix/hovercard-fastconnect branch from 4d63dd2 to 9ff9689 Compare December 16, 2024 20:56
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 Dec 16, 2024
@ninamarina ninamarina marked this pull request as ready for review December 17, 2024 15:32
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.

Hovercard changes look good, please check my comment about the popover changes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like using 'hovercard' for naming stuff inside popover component makes me feel like we're mixing two components, Hovercard should be extending the popover, but we shouldn't need to modify the popover itself

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be possible to pass in listeners from the hovercard component and then use v-sync / v-model to manage the "open" prop. Let me know if there is some blocker I am not seeing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely agree. I will do those changes in this ticket https://dialpad.atlassian.net/browse/DLT-2245

@juliodialpad
Copy link
Collaborator

Don't forgot to update the docs on the HoverCard component page

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.

I'm okay with you merging this and then fixing it later since this works, and is probably a blocker for your launch-a-thon team, but I agree with @juliodialpad that it would be ideal to have all hovercard logic in the hovercard component and we should try to do that. I realize popover is a very complicated component and it might take a while.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be possible to pass in listeners from the hovercard component and then use v-sync / v-model to manage the "open" prop. Let me know if there is some blocker I am not seeing.

Copy link

βœ”οΈ Deploy previews ready!
😎 Dialtone preview: https://dialtone.dialpad.com/deploy-previews/pr-597/
😎 Dialtone-vue 2 preview: https://dialtone.dialpad.com/vue/deploy-previews/pr-597/
😎 Dialtone-vue 3 the preview: https://dialtone.dialpad.com/vue3/deploy-previews/pr-597/

@ninamarina
Copy link
Contributor Author

I'm okay with you merging this and then fixing it later since this works, and is probably a blocker for your launch-a-thon team, but I agree with @juliodialpad that it would be ideal to have all hovercard logic in the hovercard component and we should try to do that. I realize popover is a very complicated component and it might take a while.

I created this ticket and added it to the next sprint: https://dialpad.atlassian.net/browse/DLT-2245. Thanks!

@ninamarina ninamarina dismissed juliodialpad’s stale review December 18, 2024 14:30

Created a ticket to address the suggestions

@ninamarina ninamarina merged commit e5c78a3 into staging Dec 18, 2024
10 checks passed
@ninamarina ninamarina deleted the fix/hovercard-fastconnect branch December 18, 2024 14:30
braddialpad pushed a commit that referenced this pull request Dec 19, 2024
# [9.92.0](dialtone/v9.91.1...dialtone/v9.92.0) (2024-12-18)

### Features

* **Hovercard:** DP-119841 hovercard fixes for Fast Connect ([#597](#597)) ([e5c78a3](e5c78a3))
braddialpad pushed a commit that referenced this pull request Dec 19, 2024
# [2.170.0](dialtone-vue2/v2.169.1...dialtone-vue2/v2.170.0) (2024-12-18)

### Features

* **Hovercard:** DP-119841 hovercard fixes for Fast Connect ([#597](#597)) ([e5c78a3](e5c78a3))
braddialpad pushed a commit that referenced this pull request Dec 19, 2024
# [3.163.0](dialtone-vue3/v3.162.1...dialtone-vue3/v3.163.0) (2024-12-18)

### Features

* **Hovercard:** DP-119841 hovercard fixes for Fast Connect ([#597](#597)) ([e5c78a3](e5c78a3))
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.

3 participants