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

Canvas: Improved tooltip #90162

Merged
merged 8 commits into from
Jul 10, 2024
Merged

Canvas: Improved tooltip #90162

merged 8 commits into from
Jul 10, 2024

Conversation

adela-almasan
Copy link
Contributor

@adela-almasan adela-almasan commented Jul 6, 2024

Before

Screenshot 2024-07-09 at 2 58 39 PM

After

Screen.Recording.2024-07-09.at.2.51.29.PM.mov

TODO:

  • color - for later, there are some inconsistencies that should be discussed more in depth.

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@adela-almasan adela-almasan added this to the 11.2.x milestone Jul 6, 2024
@adela-almasan adela-almasan self-assigned this Jul 6, 2024
@adela-almasan adela-almasan changed the title Canvas: Improved tooltip [WIP] Canvas: Improved tooltip Jul 6, 2024
@nmarrs
Copy link
Contributor

nmarrs commented Jul 9, 2024

Great work on this!! I am wondering if we should also consider showing the timestamp (if relevant) in the tooltip as well as we have gotten this as a request before 🤔 could be a next step though

@adela-almasan adela-almasan changed the title [WIP] Canvas: Improved tooltip Canvas: Improved tooltip Jul 9, 2024
@adela-almasan adela-almasan marked this pull request as ready for review July 9, 2024 17:28
@adela-almasan adela-almasan requested a review from a team as a code owner July 9, 2024 17:28
@adela-almasan adela-almasan requested review from leeoniya and nmarrs and removed request for a team July 9, 2024 17:28
Copy link
Contributor

@nmarrs nmarrs left a comment

Choose a reason for hiding this comment

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

Added the ability to view timestamps with data now and also removed limitation of viewing tooltips only when element is attached to datalink(s)

Great job on this!

Screen.Recording.2024-07-09.at.2.51.29.PM.mov

<div className={style.wrapper}>{renderDataLinks()}</div>
<VizTooltipHeader item={headerItem} isPinned={scene.tooltip.isOpen!} />
{element.data.text && <VizTooltipContent items={contentItems} isPinned={scene.tooltip.isOpen!} />}
<VizTooltipFooter dataLinks={element.data?.links} />
Copy link
Contributor

Choose a reason for hiding this comment

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

standardized tooltips coming in strong here 💪

@nmarrs
Copy link
Contributor

nmarrs commented Jul 9, 2024

Let's hold off on merging this until we've discussed how we want to handle which elements display a tooltip (see this internal slack discussion)

Comment on lines 284 to 285
const canShowElementTooltip = !this.isEditingEnabled;

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe something like if we go with more exclusive method for displaying tooltips

Suggested change
const canShowElementTooltip = !this.isEditingEnabled;
const isTooltipValid = (this.tooltip?.element?.data?.links?.length ?? 0) > 0 || this.tooltip?.element?.data?.field;
const canShowElementTooltip = !this.isEditingEnabled && isTooltipValid;

@adela-almasan adela-almasan merged commit 8989ac4 into main Jul 10, 2024
15 checks passed
@adela-almasan adela-almasan deleted the canvas_tooltip branch July 10, 2024 19:52
ryantxu pushed a commit that referenced this pull request Jul 11, 2024
@aangelisc aangelisc modified the milestones: 11.2.x, 11.2.0 Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants