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

Added className prop support #56

Merged
merged 5 commits into from
Feb 22, 2019
Merged

Added className prop support #56

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7e7e4c9
added className prop support
benkeen Feb 21, 2019
0116a05
code review feedback; incomplete
benkeen Feb 22, 2019
4e1825f
whitespace separator fix
benkeen Feb 22, 2019
78ce27b
Update index.d.ts
atomiks Feb 22, 2019
82fb66b
Remove failing test for now
atomiks Feb 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion demo/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,14 +117,21 @@ class ComponentChild extends React.Component {
class App extends React.Component {
state = {
arrow: false,
customClass: ''
}

toggleArrow = () => {
this.setState(state => ({
arrow: !state.arrow,
arrow: !state.arrow
}))
}

updateCustomClass = (e) => {
this.setState({
customClass: e.target.value
})
}

render() {
return (
<main className="container">
Expand Down Expand Up @@ -165,6 +172,12 @@ class App extends React.Component {
</Tippy>
</Tippy>
</Tippy>

<h1>Other</h1>
<input type="text" placeholder="Enter class" onChange={this.updateCustomClass}/>
<Tippy placement="bottom" className={this.state.customClass}>
<button>Custom class</button>
</Tippy>
Copy link
Contributor Author

@benkeen benkeen Feb 21, 2019

Choose a reason for hiding this comment

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

Not the prettiest test, but it helped to confirm the classes were being dynamically added/removed from the tippy element.

</main>
)
}
Expand Down
1 change: 1 addition & 0 deletions index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ export interface TippyProps extends Omit<Props, 'content'> {
onCreate?: (tip: Instance) => void
isVisible?: boolean
isEnabled?: boolean
className?: string
}

declare const Tippy: React.ForwardRefExoticComponent<TippyProps>
Expand Down
26 changes: 23 additions & 3 deletions src/Tippy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import React, {
useState,
useRef,
useEffect,
useLayoutEffect,
} from 'react'
import { createPortal } from 'react-dom'
import PropTypes from 'prop-types'
Expand All @@ -13,6 +14,7 @@ import {
hasOwnProperty,
ssrSafeCreateDiv,
preserveRef,
updateClassName,
} from './utils'

function Tippy(props) {
Expand All @@ -33,12 +35,17 @@ function Tippy(props) {
useEffect(() => {
instanceRef.current = tippy(targetRef.current, options)

const { onCreate, isEnabled, isVisible } = props
const { onCreate, isEnabled, isVisible, className } = props

if (onCreate) {
onCreate(instanceRef.current)
}

if (className) {
const { tooltip } = instanceRef.current.popperChildren
updateClassName(tooltip, 'add', props.className)
}

if (isEnabled === false) {
instanceRef.current.disable()
}
Expand All @@ -55,7 +62,7 @@ function Tippy(props) {
}
}, [])

useEffect(() => {
useLayoutEffect(() => {
if (!isMounted) {
return
}
Expand All @@ -70,7 +77,6 @@ function Tippy(props) {
if (isEnabled === false) {
instanceRef.current.disable()
}

if (isVisible === true) {
instanceRef.current.show()
}
Expand All @@ -79,6 +85,19 @@ function Tippy(props) {
}
})

useLayoutEffect(() => {
if (!isMounted) {
return
}

const { tooltip } = instanceRef.current.popperChildren
updateClassName(tooltip, 'add', props.className)

return () => {
updateClassName(tooltip, 'remove', props.className)
}
}, [props.className])

return (
<>
{cloneElement(props.children, {
Expand All @@ -99,6 +118,7 @@ Tippy.propTypes = {
onCreate: PropTypes.func,
isVisible: PropTypes.bool,
isEnabled: PropTypes.bool,
className: PropTypes.string,
}

Tippy.defaultProps = {
Expand Down
10 changes: 9 additions & 1 deletion src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
export function getNativeTippyProps(props) {
// eslint-disable-next-line no-unused-vars
const { children, onCreate, isVisible, isEnabled, ...nativeProps } = props
const { children, onCreate, isVisible, isEnabled, className, ...nativeProps } = props
return nativeProps
}

Expand All @@ -22,3 +22,11 @@ export function preserveRef(ref, node) {
export function ssrSafeCreateDiv() {
return typeof document !== 'undefined' && document.createElement('div')
}

export function updateClassName(tooltip, action, classNames) {
classNames.split(/\s+/).forEach(name => {
if (name) {
tooltip.classList[action](name)
}
})
}
23 changes: 23 additions & 0 deletions test/Tippy.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,29 @@ describe('<Tippy />', () => {
expect(tip.popper.querySelector('strong')).not.toBeNull()
})

test('custom class name get added to DOM', () => {
const className = 'hello'
const { container } = render(
<Tippy content="tip content" className={className}>
Copy link
Owner

@atomiks atomiks Feb 21, 2019

Choose a reason for hiding this comment

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

Can you add a test that checks the add/remove when updating the state? e.g. starts with className A, after updating, ends with className B, but doesn't still contain className A. the useEffect looks correct to me anyway but still :-)

Copy link
Contributor Author

@benkeen benkeen Feb 22, 2019

Choose a reason for hiding this comment

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

I'm afraid I may need a hand here. Two things, actually:

  • from what I've read, useLayoutEffect does seem to be the right method to choose for this, but it throws a warning when running the test: "Warning: useLayoutEffect does nothing on the server, because its effect cannot be encoded..." (see full message: https://github.com/facebook/react/pull/14596/files). I'm not sure how to suppress that. Or possibly it's just not working properly when run in the test.
  • secondly, I couldn't get the test to work as expected (Possibly it's actually a bug in the code?). Rerendering the component shows the old class didn't get removed - but testing manually in the browser seems okay.

Copy link
Owner

@atomiks atomiks Feb 22, 2019

Choose a reason for hiding this comment

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

To be honest, I don't really know either. There seems to be missing documentation for this or maybe I can't find it??? 😢

Copy link
Contributor

@KubaJastrz KubaJastrz Feb 22, 2019

Choose a reason for hiding this comment

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

useLayoutEffect runs synchronously after each render, so it's good for things like DOM manipulation outside React (like plain DOM api, or jQuery). I'm gonna take a look at this PR today

Copy link
Contributor

@KubaJastrz KubaJastrz Feb 22, 2019

Choose a reason for hiding this comment

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

I changed it back to useEffect and it seems like rerender() doesn't invoke useEffect cleanup function. However, React in browser environment does, so that's why it is working when testing manually.

I have no idea how to fix it either.

Copy link
Owner

@atomiks atomiks Feb 22, 2019

Choose a reason for hiding this comment

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

I will go ahead and merge this since it's working fine in the browser and logic looks correct 🤷‍♂️ If we can figure out how to test it, make a PR

<button />
</Tippy>,
)
const tip = container.querySelector('button')._tippy
expect(tip.popper.querySelector(`.${className}`)).not.toBeNull()
})

test('custom class name get added to DOM', () => {
const classNames = 'hello world'
const { container } = render(
<Tippy content="tip content" className={classNames}>
<button />
</Tippy>,
)
const tip = container.querySelector('button')._tippy
expect(tip.popper.querySelector('.hello')).not.toBeNull()
expect(tip.popper.querySelector('.world')).not.toBeNull()
})

test('unmount destroys the tippy instance and allows garbage collection', () => {
const { container, unmount } = render(
<Tippy content="tooltip">
Expand Down