Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/validation-animation-derive-in-render.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': patch
---

FormControl: Avoid an extra re-render when showing a validation message.
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import {render} from '@testing-library/react'
import {describe, it, expect} from 'vitest'
import ValidationAnimationContainer from './ValidationAnimationContainer'
import {createRenderCounter} from '../../utils/testing/profiler'
import {implementsClassName} from '../../utils/testing'

describe('ValidationAnimationContainer', () => {
implementsClassName(props => <ValidationAnimationContainer show {...props} />)

it('renders children when show is true', () => {
const {getByText} = render(<ValidationAnimationContainer show>content</ValidationAnimationContainer>)
expect(getByText('content')).toBeInTheDocument()
})

it('does not render children when show is false initially', () => {
const {queryByText} = render(<ValidationAnimationContainer show={false}>content</ValidationAnimationContainer>)
expect(queryByText('content')).not.toBeInTheDocument()
})

it('renders children after show transitions to true', () => {
const {queryByText, rerender} = render(
<ValidationAnimationContainer show={false}>content</ValidationAnimationContainer>,
)
expect(queryByText('content')).not.toBeInTheDocument()

rerender(<ValidationAnimationContainer show={true}>content</ValidationAnimationContainer>)
expect(queryByText('content')).toBeInTheDocument()
})

it('commits once (no cascade) when show transitions to true', () => {
// The previous implementation set `shouldRender` from an effect, forcing an
// extra commit after the prop change. Deriving it during render keeps it to one.
const [Wrap, counter] = createRenderCounter()
const {rerender} = render(
<Wrap>
<ValidationAnimationContainer show={false}>content</ValidationAnimationContainer>
</Wrap>,
)
counter.reset()

rerender(
<Wrap>
<ValidationAnimationContainer show={true}>content</ValidationAnimationContainer>
</Wrap>,
)

expect(counter.updateCount).toBe(1)
expect(counter.nestedUpdateCount).toBe(0)
})
})
Original file line number Diff line number Diff line change
@@ -1,25 +1,26 @@
import type {HTMLProps} from 'react'
import type React from 'react'
import {useEffect, useState} from 'react'
import {useState} from 'react'
import classes from './ValidationAnimationContainer.module.css'

interface Props extends HTMLProps<HTMLDivElement> {
show?: boolean
}
const ValidationAnimationContainer: React.FC<React.PropsWithChildren<Props>> = ({show, children}) => {
const ValidationAnimationContainer: React.FC<React.PropsWithChildren<Props>> = ({show, children, style, ...rest}) => {
const [shouldRender, setRender] = useState(show)

useEffect(() => {
// eslint-disable-next-line react-hooks/set-state-in-effect, react-you-might-not-need-an-effect/no-derived-state, react-you-might-not-need-an-effect/no-event-handler
if (show) setRender(true)
}, [show])
// Start rendering as soon as `show` becomes true. Adjusting state during render
// (instead of from an effect) avoids the extra post-commit render the effect caused.
if (show && !shouldRender) {
setRender(true)
}

const onAnimationEnd = () => {
if (!show) setRender(false)
}

return shouldRender ? (
<div style={{height: show ? 'auto' : 0, overflow: 'hidden'}}>
<div {...rest} style={{...style, height: show ? 'auto' : 0, overflow: 'hidden'}}>
<div data-show={show ? '' : undefined} onAnimationEnd={onAnimationEnd} className={classes.Animation}>
{children}
</div>
Expand Down
Loading