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

Tooltip in the Sidepanel for cut-off layer name Text #297

Merged
merged 7 commits into from
Sep 5, 2024

Conversation

kartik-raj7
Copy link
Contributor

Description

Tooltip for concatenated layer name text

What is the purpose of this pull request?

  • New feature
  • Documentation update
  • Bug fix
  • Refactor
  • Release
  • Other

@drfarrell
Copy link
Collaborator

Wooo excited about this one!

Copy link
Contributor

@Kitenite Kitenite left a comment

Choose a reason for hiding this comment

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

I also would increase the length of the text in textContent so that the tooltip has more information like so

09ce88b#diff-66c17179f449d824754ee054672c58937e6bd0d9416f38e76a9c8929b88fc03cL232

@@ -45,7 +45,7 @@ const LayersPanel = observer(() => {
<PinLeftIcon />
</button>
</TabsList>
<Separator className="mt-1" />
<Separator className="mt-1" id="LayersPanelDiv" />
Copy link
Contributor

Choose a reason for hiding this comment

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

I would move the id to the tab container since we can use it to calculate the width offset
09ce88b#diff-f0245d5cb38ab89312972b0120a231f7e191c7938935b2c5dd18f4c3cddbfe08R49

{node.data.textContent}
</span>
</TooltipTrigger>
{isOverflowing && (
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just show the tooltip if there's text content for simplicity
09ce88b#diff-eced7d84f55e60e45f2876a4fc40b812bb12b845d5b5cdb4d641a619346d96d1R181

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we'll need it for every type of layer

image

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be applying to it all @drfarrell

</span>
</TooltipTrigger>
{isOverflowing && (
<TooltipPortal container={document.getElementById('LayersPanelDiv')}>
Copy link
Contributor

@Kitenite Kitenite Sep 5, 2024

Choose a reason for hiding this comment

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

You can use sideOffset to move the tooltip position based on the width of the container. Since the problem is that the node is overflowing, we can calculate the offset as the containerWidth - nodeWidth - scrollPosition

8405200#diff-eced7d84f55e60e45f2876a4fc40b812bb12b845d5b5cdb4d641a619346d96d1R81

@Kitenite
Copy link
Contributor

Kitenite commented Sep 5, 2024

Overall, nice work! You had most of it going, just needed to adjust the tooltip position and some small improvements

(I opened a draft PR on top of yours for reference since I can't push to your branch)
https://github.com/onlook-dev/onlook/pull/298/commits

@drfarrell
Copy link
Collaborator

My recommendation is to make it a max width of 200px, and restrict it to 4 lines of text before going ...

For example:

So it should look a little something
like this if you were to see the full
length of text that would occupy a
tooltip of the size you would exp...

@Kitenite Kitenite mentioned this pull request Sep 5, 2024
6 tasks
@kartik-raj7
Copy link
Contributor Author

kartik-raj7 commented Sep 5, 2024

My recommendation is to make it a max width of 200px, and restrict it to 4 lines of text before going ...

For example:

So it should look a little something like this if you were to see the full length of text that would occupy a tooltip of the size you would exp...

image

Does this look Good!

@Kitenite
Copy link
Contributor

Kitenite commented Sep 5, 2024

Seems like the bottom padding is a little smaller?

@Kitenite Kitenite self-requested a review September 5, 2024 19:33
Copy link
Contributor

@Kitenite Kitenite left a comment

Choose a reason for hiding this comment

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

Awesome work!

@Kitenite Kitenite merged commit 75b16ec into onlook-dev:main Sep 5, 2024
@Kitenite Kitenite linked an issue Sep 7, 2024 that may be closed by this pull request
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.

[FEAT] Tooltip for concatenated layer name text
4 participants