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): allow multiple tooltips per grid cell #1448

Merged
merged 9 commits into from
Mar 31, 2024

Conversation

ghiscoding
Copy link
Owner

@ghiscoding ghiscoding commented Mar 29, 2024

  • prior to this PR, we could only have 1 tooltip per cell, but in some occasions, we might have a cell with multiple icons & tooltips, if that happens then we should have separate tooltips (with their dedicated & different tooltipt text) positioned on that inner icon

TODOs

  • possibly reposition the arrow at the mouse location?
  • possibly center tooltip with center of target element?
  • test with multiple tooltips for header row/filter row elements as well
    • this actually requires new SlickEvents because the existing events for header/headerRow are using mouseenter but we need mouseover to differentiate multiple tooltip within the same header (mouseenter only gets triggered once while mouseover gets triggered multiple times while hovering any of element within container). So instead of possible chaning the previous behavior, I decided to duplicate onHeaderMouseEnter to onHeaderMouseOver and the same goes for onHeaderRowMouseEnter and also ...MouseOut for both header types too (so adding 4 new events to cover all use cases). The funny thing is that the event for the cell onMouseEnter is actually using mouseover which is not actually matching with its name.
  • add new option to align tooltip to target element (default to false unless we have multiple tooltips, in that case it's always true)
  • unit test coverage
  • E2E test coverage

for now the POC is mostly working, note that I purposely set a negative top margin in order to also test tooltip that can sometime show at the bottom (in 95% of the case it's showing on top except when there's not enough space on top)

Example 22 (align right)

brave_9GJWaisxNl

Example 11 (align left)

brave_3DHk9zGNbF

Example 12 (multiple tooltips in header row)

brave_lvAr9wqtJK

existing bugs

  • found existing bug when opening slider editor (not a new bug, this one existed before opening this PR)
    brave_AvWeTzevAt
  • another bug when tooltip text is too wide, we should use text overflow ellipsis
    image

- prior to this PR, we could only have 1 tooltip per cell, but in some occasions, we might have a cell with multiple icons & tooltips, if that happens then we should have separate tooltips (with their dedicated & different tooltipt text) positioned on that inner icon
Copy link

stackblitz bot commented Mar 29, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@ghiscoding
Copy link
Owner Author

ghiscoding commented Mar 29, 2024

@zewa666 I got a POC mostly working, however I did find some separate/existing bugs with the tooltip plugin and I have identified a few things to potentially do and new options to add in the TODOs list above. Feel free to provide feedback, you could also give it a try by using this awesome new individual PR Stackblitz link.

Copy link

codecov bot commented Mar 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.8%. Comparing base (863933f) to head (2c3e67f).

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1448     +/-   ##
========================================
+ Coverage    99.8%   99.8%   +0.1%     
========================================
  Files         199     199             
  Lines       21621   21682     +61     
  Branches     7236    7123    -113     
========================================
+ Hits        21557   21618     +61     
+ Misses         64      58      -6     
- Partials        0       6      +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

- that required to add 4 new SlickEvent for header mouseover (instead of mouseenter which is not sufficient)
- Example 16 tooltip wasn't showing progress bar html, this was a regression that arrived after converted all Formatters to native HTML instead of returning HTML string
- also fixed an issue with long text that is larger than the div width, the ellipsis should show but it wasn't. We can fix this by adding an inner div for the text body and move the text into it and show the ellipsis on that inner div instead of the full tooltip container
- also fix tooltip text color for dark mode
- also fix tooltip text color for dark mode
@ghiscoding ghiscoding merged commit 061c4a0 into master Mar 31, 2024
9 checks passed
@ghiscoding ghiscoding deleted the bugfix/multiple-tooltips branch March 31, 2024 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant