Skip to content
This repository was archived by the owner on Nov 9, 2024. It is now read-only.

Conversation

@inxilpro
Copy link
Contributor

This addresses #132 — it hasn't been fully tested, but I just wanted to push up what I have for any feedback/discussion. The basic usage is:

import Tippy, {useSingleton} from '@tippy.js/react';

function Demo() {
  const singleton = useSingleton({delay: 500});

  return (
    <div>
      <Tippy singleton={singleton} content="Tooltip 1">
        <button>One</button>
      </Tippy>
      <Tippy singleton={singleton} content="Tooltip 2">
        <button>Two</button>
      </Tippy>
      <Tippy singleton={singleton} content="Tooltip 3">
        <button>Three</button>
      </Tippy>
    </div>
  );
}

@atomiks
Copy link
Owner

atomiks commented Oct 10, 2019

What benefits does this have over the filtering technique I mentioned in the issue? e.g.

<TippySingleton>
  <div />
  <Tippy content="tooltip">
    <button />
  </Tippy>
  <div />
  <Tippy content="tooltip">
    <button />
  </Tippy>
  <div />
</TippySingleton>
// If `content` isn't enough (e.g. other children have this prop),
// fallback to them specifying `singleton` as a prop to make it work
// Or is there a better way?
function isTippyChild(child) {
  return child.props.content || child.props.singleton;
}

Ok - this hook will also work if one of the <Tippy /> is nested within another component or element as well, but the above won't.

Seems necessary to add as a feature.

@inxilpro
Copy link
Contributor Author

Yeah, basically this allows us to group tippy instances regardless of where they are in our component tree. In our case, I may be able to restructure things such that they’re all children of the same container, but it’s not convenient to do so. Using the hook means it doesn’t matter where your Tippy components live in your app…

@atomiks
Copy link
Owner

atomiks commented Oct 10, 2019

I'm just wondering if we should instead add a render props API to <TippySingleton /> instead of a hook?

I know the whole purpose of hooks was to reduce nesting in component trees, but in this case, I dislike how there would be "two ways" to do things in terms of importing functionality 😖

@atomiks
Copy link
Owner

atomiks commented Oct 10, 2019

The story on the README might be something like:

Singleton

Wraps the createSingleton() method.

  1. If each of your reference elements are adjacent to one another with no nesting in the tree, import the <TippySingleton /> component:
import Tippy, {TippySingleton} from '@tippy.js/react';

// ...

<TippySingleton delay={500}>
  <Tippy content="a">
    <button />
  </Tippy>
  <Tippy content="b">
    <button />
  </Tippy>
</TippySingleton>
  1. If your reference elements are not adjacent, or there is nesting in the tree, import the useSingleton hook:
import Tippy, {useSingleton} from '@tippy.js/react';

function App() {
  const singleton = useSingleton({delay: 500});

  return (
    <>
      <Tippy content="a" singleton={singleton}>
        <button />
      </Tippy>
      <button />
      <div>
        <Tippy content="b" singleton={singleton}>
          <button />
        </Tippy>
      </div>
    </>
  );
}

@inxilpro
Copy link
Contributor Author

inxilpro commented Oct 10, 2019

I did consider using Context, would essentially mean converting TippySingleton to a context provider, and then have all Tippy instances check for that context internally. It would work with nested components without adding a new API, but my preference is for the hook as it feels more natural to me.

@inxilpro
Copy link
Contributor Author

I do also think that offering 2 ways is OK, especially right now, since some people like the hooks API and some prefer classes and render props.

Personally, if I never have to deal with another render prop, that will make me very happy :)

@atomiks
Copy link
Owner

atomiks commented Oct 10, 2019

I think the hook is fine =]

As for tests - the <TippySingleton /> tests are far from robust or well written, but hopefully you can write some decent ones for it?

@inxilpro
Copy link
Contributor Author

@atomiks Alright, I replicated all the tests in TippySingleton.test.js as their useSingleton counterparts. All the tests pass, and you can take a look at it in the demo/ as well. Let me know if you have any other concerns…

@atomiks
Copy link
Owner

atomiks commented Oct 11, 2019

Thanks a lot!

What is onComponent? createSingleton also has onCreate prop which should cover that

@inxilpro
Copy link
Contributor Author

It’s basically there for testing only. I’m using it to access the singleton instance and the array of instances in the test. It’s not ideal, but it feels necessary.

@atomiks
Copy link
Owner

atomiks commented Oct 11, 2019

It’s basically there for testing only. I’m using it to access the singleton instance and the array of instances in the test. It’s not ideal, but it feels necessary.

Yeah you're right 😳 Maybe it should be underscored and highlighted it's only for testing?

@inxilpro
Copy link
Contributor Author

Yeah, maybe I’ll rename it to __dangerouslyWatchCommponentRef or something. I’m on my phone right now but will update my PR in a bit.

@inxilpro
Copy link
Contributor Author

Actually, I was able to remove that method entirely and get all the instance references I needed with wrapper components.

@atomiks
Copy link
Owner

atomiks commented Oct 11, 2019

Edit: nvm made some tweaks myself

This looks good now, thanks a lot for this!

@inxilpro
Copy link
Contributor Author

Cool, I like your tweaks to the README!

@atomiks atomiks merged commit 80a3f9a into atomiks:master Oct 11, 2019
@inxilpro
Copy link
Contributor Author

Thank you for all your hard work on Tippy—and especially the fantastic documentation. It's a great library.

@atomiks
Copy link
Owner

atomiks commented Oct 11, 2019

Just realized –– does this hook need types? /cc @KubaJastrz

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants