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

**Feature:** Splash hooks & useInterval #940

Merged
merged 22 commits into from
Mar 11, 2019
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
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
110 changes: 57 additions & 53 deletions src/Splash/Splash.Animation.tsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import * as React from "react"

import React, { useState } from "react"
import useInterval from "../useInterval"
import useWindowSize from "../useWindowSize"
import styled from "../utils/styled"

export interface Props {
fullscreen?: boolean
size?: number
}

Expand Down Expand Up @@ -34,73 +36,75 @@ const bounce = (coord: number): number => {
return coord
}

const Container = styled("div")<{ size: number }>(({ size }: { size: number }) => ({
width: size,
height: size,
const Container = styled("div")({
position: "absolute",
top: "50%",
left: "50%",
transform: "translate3d(-50%, -50%, 0)",
}))
})

/// Move highly highly dynamic style out of css-js to prevent uneeded classname generation
JoshRosenstein marked this conversation as resolved.
Show resolved Hide resolved

const Box = styled("div")<{ x: number; y: number }>(({ x, y }) => ({
const Box = styled("div")({
position: "absolute",
transition: "all 0.5s ease-in-out",
top: `calc(${(x / (squares - 1)) * 100}% + 4px)`,
left: `calc(${(y / (squares - 1)) * 100}% + 4px)`,

borderRadius: 6,
width: `calc(${100 / (squares - 1)}% - 8px)`,
height: `calc(${100 / (squares - 1)}% - 8px)`,
backgroundColor: "rgba(255, 255, 255, 0.06)",
}))

class Animation extends React.Component<Props, State> {
public state = {
animationStep: 0,
coordinates: Array.apply(null, { length: boxes })
.map(Number.call, Number)
.map(() => ({ x: integerRandom(squares), y: integerRandom(squares) })),
}
})

public animationInterval?: number

// Shift the coordinate of every third tile in a random direction.
// Each animation shifts a different set of tiles.
public shiftSomeTiles() {
this.setState(prevState => ({
animationStep: prevState.animationStep + 1,
coordinates: prevState.coordinates.map((coord: { x: number; y: number }, index: number) => {
if (index % 3 === prevState.animationStep % 3) {
const dx = integerRandom(3) - 1
const dy = integerRandom(3) - 1
return {
x: bounce(coord.x + dx),
y: bounce(coord.y - dy),
}
}
return coord
}),
}))
}
const initialState = {
animationStep: 0,
coordinates: Array.apply(null, Array(boxes))
.map((_: any, i: number) => i) // Fixes typescript Error
JoshRosenstein marked this conversation as resolved.
Show resolved Hide resolved
.map(() => ({ x: integerRandom(squares), y: integerRandom(squares) })),
}

public componentDidMount() {
this.animationInterval = window.setInterval(this.shiftSomeTiles.bind(this), 5000)
}
const Animation: React.FC<Props> = ({ fullscreen, size }) => {
let containerSize = size || 600
JoshRosenstein marked this conversation as resolved.
Show resolved Hide resolved

public componentWillUnmount() {
clearInterval(this.animationInterval)
if (fullscreen) {
JoshRosenstein marked this conversation as resolved.
Show resolved Hide resolved
const windowSize = useWindowSize()
containerSize = Math.max(windowSize.width, windowSize.height) - 20
}

public render() {
const size = this.props.size || 600
return (
<Container size={size}>
{this.state.coordinates.map((coord: { x: number; y: number }, index: number) => (
<Box key={index} x={coord.x} y={coord.y} />
))}
</Container>
)
}
const [state, updateAnimation] = useState<State>(initialState)
useInterval(
() => {
updateAnimation({
animationStep: state.animationStep + 1,
coordinates: state.coordinates.map((coord: { x: number; y: number }, index: number) => {
TejasQ marked this conversation as resolved.
Show resolved Hide resolved
if (index % 3 === state.animationStep % 3) {
const dx = integerRandom(3) - 1
const dy = integerRandom(3) - 1
return {
x: bounce(coord.x + dx),
y: bounce(coord.y - dy),
}
}
return coord
}),
})
},
5000,
true,
)

return (
<Container style={{ width: containerSize, height: containerSize }}>
{state.coordinates.map((coord: { x: number; y: number }, index: number) => (
<Box
key={index}
style={{
top: `${(coord.x / (squares - 1)) * 100}%`,
left: `${(coord.y / (squares - 1)) * 100}%`,
}}
/>
))}
</Container>
)
}

export default Animation
58 changes: 17 additions & 41 deletions src/Splash/Splash.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import * as React from "react"

import { readableTextColor } from "../utils"
import { expandColor, OperationalStyleConstants } from "../utils/constants"

JoshRosenstein marked this conversation as resolved.
Show resolved Hide resolved
import styled from "../utils/styled"
import Animation from "./Splash.Animation"
import OperationalLogo from "./Splash.Logo"
Expand Down Expand Up @@ -89,47 +90,22 @@ const Static = styled("div")`
}
`

class Splash extends React.Component<SplashProps, Readonly<State>> {
public readonly state = {
animationSize: Math.max(window.innerWidth, window.innerHeight),
}

public static defaultProps = {
color: "#fff",
}

public handleResize = () => {
this.setState({
animationSize: Math.max(window.innerWidth, window.innerHeight) as number,
})
}

public componentDidMount() {
window.addEventListener("resize", this.handleResize)
}

public componentWillUnmount() {
window.removeEventListener("resize", this.handleResize)
}

public render() {
const { logo, color } = this.props
return (
<Container color={color}>
<Animation size={this.state.animationSize} />
<Content color={color}>
<TitleBar>
<OperationalLogo size={110} logo={logo} />
<TitleBarContent>
<h1>{this.props.title}</h1>
<div>{this.props.actions}</div>
</TitleBarContent>
</TitleBar>
<Static>{this.props.children}</Static>
</Content>
</Container>
)
}
const Splash: React.FC<SplashProps> = ({ color, logo, children, title, actions }) => {
return (
<Container color={color}>
<Animation fullscreen />
<Content color={color}>
<TitleBar>
<OperationalLogo size={110} logo={logo} />
<TitleBarContent>
<h1>{title}</h1>
<div>{actions}</div>
</TitleBarContent>
</TitleBar>
<Static>{children}</Static>
</Content>
</Container>
)
}

export default Splash
18 changes: 18 additions & 0 deletions src/useInterval/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
## Usage

This hook sets up an interval and clears it after unmounting

```jsx
const MyComponent = () => {
let [count, setCount] = React.useState(0)

useInterval(() => {
// Your custom logic here
setCount(count + 1)
}, 1000)

return <h1>{count}</h1>
}

;<MyComponent />
```
37 changes: 37 additions & 0 deletions src/useInterval/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import { useEffect, useRef } from "react"
Copy link
Contributor

@stereobooster stereobooster Feb 28, 2019

Choose a reason for hiding this comment

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

The simplest implementation would be

import { useEffect } from 'react';

const useInterval = (fn: () => void, delay: number) => {
  useEffect(() => {
    const id = setInterval(fn, delay)
    return () => clearInterval(id)
  })
};

export default useInterval;

UPD code works only because useEffect will be triggered on each render (which is wrong)

  useEffect(() => {
    const id = setInterval(fn, delay)
    return () => clearInterval(id)
-  })
+  }, [delay])

if we would add delay, it would brake. Example of the bug

const useInterval = (fn: () => void, delay: number) => {
  useEffect(() => {
    const id = setInterval(() => {
      console.log(1);
      fn();
    }, delay)
    return () => clearInterval(id)
  }, [delay])
};

function Counter() {
  const [count, setCount] = useState(0);
  useInterval(() => {
    // Your custom logic here
    setCount(count + 1);
  }, 1000);
  return <h1>{count}</h1>;
}

count will change from 0 to 1 only once (because count is locally bound to function passed to useInterval), but console.log(1); will be called many times

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stereobooster ,Dan wrote a nice piece on this.

Simple Implementation works for simple cases ( and how its currently being used in Splash)

But this version is more scalable/reusable ...

"But setInterval() does not “forget”. It will forever reference the old props and state until you replace it — which you can’t do without resetting the time."

Where useRef comes into play...

"The problem boils down to this:

  • We do setInterval(callback1, delay) with callback1 from first render.
  • We have callback2 from next render that closes over fresh props and state.
  • But we can’t replace an already existing interval without resetting the time!

So what if we didn’t replace the interval at all, and instead introduced a mutable savedCallback variable pointing to the latest interval callback?"

"This mutable savedCallback needs to “persist” across the re-renders. So it can’t be a regular variable. We want something more like an instance field."

For Example, this allows us to change the delay or callback between renders without resetting the time

But if you want be to change to simple version, i will do that!

Copy link
Contributor

Choose a reason for hiding this comment

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

My concern is that the code should be clear, so people after us would be able to read it easily and change it if they want to. It takes quite some iterations to understand what happens here. After reading Dan's article this all start to make sense, but before it seemed over-engineered. So adding link to the Dan's article and maybe some more comments would help

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree. @JoshRosenstein please add some documentation if you wish to keep the component the way it is. ❤️

Copy link
Contributor

Choose a reason for hiding this comment

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

@JoshRosenstein have you seen these comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mpotomin - It was my understanding the "simplest implementation" version breaks, and should keep the current useInterval as is, and i added additional comments since, Did i miss something ? Or just forgot to resolve?

Edit useInterval Test

Copy link
Contributor

Choose a reason for hiding this comment

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

You just forgot to acknowledge. Thanks for responding, @JoshRosenstein! @stereobooster is that okay with you?


// Ref: https://github.com/Hermanya/use-interval/blob/master/src/index.tsx

// tslint:disable-next-line
JoshRosenstein marked this conversation as resolved.
Show resolved Hide resolved
const noop = () => {}
JoshRosenstein marked this conversation as resolved.
Show resolved Hide resolved

/**
* useInterval
JoshRosenstein marked this conversation as resolved.
Show resolved Hide resolved
*/
TejasQ marked this conversation as resolved.
Show resolved Hide resolved
export function useInterval(callback: () => void, delay: number | null, immediate?: boolean) {
TejasQ marked this conversation as resolved.
Show resolved Hide resolved
const savedCallback = useRef(noop)
JoshRosenstein marked this conversation as resolved.
Show resolved Hide resolved

// Remember the latest callback.
JoshRosenstein marked this conversation as resolved.
Show resolved Hide resolved
useEffect(() => {
savedCallback.current = callback
})

// Execute callback if immediate is set.
useEffect(() => {
JoshRosenstein marked this conversation as resolved.
Show resolved Hide resolved
if (immediate && delay !== null) {
savedCallback.current()
}
}, [immediate])

// Set up the interval.
useEffect(() => {
if (delay === null) {
return undefined
}
const tick = () => savedCallback.current()
const id = setInterval(tick, delay)
JoshRosenstein marked this conversation as resolved.
Show resolved Hide resolved
return () => clearInterval(id)
}, [delay])
}

export default useInterval
75 changes: 75 additions & 0 deletions src/useInterval/useInterval.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
import React from "react"
import { cleanup, render } from "react-testing-library"

import useInterval, { useInterval as namedHook } from "."

// ref https://github.com/Hermanya/use-interval/blob/master/src/test.tsx

JoshRosenstein marked this conversation as resolved.
Show resolved Hide resolved
beforeEach(cleanup)

const MyComponent: React.FC<{ fn: () => void; delay: number | null; immediate?: boolean }> = ({
fn,
delay,
immediate,
}) => {
useInterval(fn, delay, immediate)
return <span>A Interval is running </span>
JoshRosenstein marked this conversation as resolved.
Show resolved Hide resolved
}

describe("useInterval", () => {
it("exports a function (default)", () => {
expect(useInterval).toBeInstanceOf(Function)
})
it("exports a function (named)", () => {
expect(namedHook).toBeInstanceOf(Function)
})
it("hooks identity are the same", () => {
expect(useInterval).toBe(namedHook)
})
it("is a named function", () => {
// aids stack trace debugging.
expect(useInterval.name).toBe("useInterval")
})

describe("regular mode (delayed)", () => {
jest.useFakeTimers()

const fn: () => void = jest.fn()
const { container } = render(<MyComponent fn={fn} delay={500} />)

expect(fn).toBeCalledTimes(0 /* not called on first render */)
jest.advanceTimersByTime(500)
expect(fn).toBeCalledTimes(1)
jest.advanceTimersByTime(1500)
expect(fn).toBeCalledTimes(4)

test("cancels interval when delay is null", () => {
render(<MyComponent immediate fn={fn} delay={null} />, { container })
jest.advanceTimersByTime(1500)
expect(fn).toBeCalledTimes(4)
})

jest.clearAllTimers()
})

describe("immediate mode", () => {
jest.useFakeTimers()

const fn = jest.fn()
const { container } = render(<MyComponent immediate fn={fn} delay={500} />)

expect(fn).toBeCalledTimes(1 /* called immediatelly on render */)
jest.advanceTimersByTime(500)
expect(fn).toBeCalledTimes(2)
jest.advanceTimersByTime(1500)
expect(fn).toBeCalledTimes(5)

it("cancels interval when delay is null", () => {
render(<MyComponent immediate fn={fn} delay={null} />, { container })
jest.advanceTimersByTime(1500)
expect(fn).toBeCalledTimes(5)
})

jest.clearAllTimers()
})
})