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

Copy Tooltip from nns-dapp #367

Merged
merged 5 commits into from
Jan 25, 2024
Merged

Copy Tooltip from nns-dapp #367

merged 5 commits into from
Jan 25, 2024

Conversation

dskloetd
Copy link
Collaborator

Motivation

NNS dapp has a tooltip component with somewhat broken behavior.
Before fixing it, I thought it would be useful to have a showcase in gix-components to easily play around with it while changing it.
And it seemed like the kind of component that would be useful to have in gix-components anyway.

So before fixing the tooltip component, I'm copying it unchanged from nns-dapp to gix-components.

Once this component is fixed, we can switch nns-dapp over to this tooltip component.

Changes

  1. Copied Tooltip.svelte, Tooltip.spec.ts and TooltipTest.svelte from the nns-dapp repo unchanged.
  2. Added a page with showcase for the component.

Screenshots

image

@dskloetd dskloetd marked this pull request as ready for review January 25, 2024 13:54
Copy link
Collaborator

@lmuntaner lmuntaner left a comment

Choose a reason for hiding this comment

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

It would be good to add an e2e test, which are easy to add here.

@dskloetd
Copy link
Collaborator Author

It would be good to add an e2e test, which are easy to add here.

Done

@dskloetd dskloetd requested a review from lmuntaner January 25, 2024 15:27
Copy link
Collaborator

@lmuntaner lmuntaner left a comment

Choose a reason for hiding this comment

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

We could also have a screenshot once we fix the issue, right?

@dskloetd
Copy link
Collaborator Author

We could also have a screenshot once we fix the issue, right?

What do you mean? Did you not see the screenshot?

@dskloetd dskloetd merged commit 18fb763 into main Jan 25, 2024
8 checks passed
@dskloetd dskloetd deleted the kloet/tooltip branch January 25, 2024 16:40
@lmuntaner
Copy link
Collaborator

We could also have a screenshot once we fix the issue, right?

What do you mean? Did you not see the screenshot?

Sorry, I meant a screenshot testing of the issue once it's fixed.

@peterpeterparker peterpeterparker linked an issue Jan 26, 2024 that may be closed by this pull request
@dskloetd
Copy link
Collaborator Author

Actually, the e2e test I added observes the issue.
The screenshot when taken on Mac looks like the screenshot in the PR description.
But on Linux it somehow doesn't trigger.
My guess is that the button is right in the center and through some kind of rounding error is considered to be left of center on Linux and right of center on Mac or something.

But once it's fixed, the issue of course won't be visible in the screenshot because then the issue doesn't exist anymore.
So I'm still not sure what kind of screenshot you expect after fixing the isuse.

@lmuntaner
Copy link
Collaborator

Actually, the e2e test I added observes the issue. The screenshot when taken on Mac looks like the screenshot in the PR description. But on Linux it somehow doesn't trigger. My guess is that the button is right in the center and through some kind of rounding error is considered to be left of center on Linux and right of center on Mac or something.

But once it's fixed, the issue of course won't be visible in the screenshot because then the issue doesn't exist anymore. So I'm still not sure what kind of screenshot you expect after fixing the isuse.

I would expect a working screenshot that in case we break it again we find it early.

@dskloetd
Copy link
Collaborator Author

Oh, I see. I don't think that's what screenshots are for. And also it's more likely that some cases we didn't think of are still/already broken rather than that some obvious case break. And it would be too many screenshots to cover even a decent fraction of cases.

The right way forward is to factor the logic outside of the component and have lots of unit tests with numbers.

@lmuntaner
Copy link
Collaborator

Oh, I see. I don't think that's what screenshots are for. And also it's more likely that some cases we didn't think of are still/already broken rather than that some obvious case break. And it would be too many screenshots to cover even a decent fraction of cases.

The right way forward is to factor the logic outside of the component and have lots of unit tests with numbers.

If we can make it as unit tests better, yes.

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: move <Hash /> and <Tooltip /> components to gix-cmp
2 participants