Skip to content

Commit

Permalink
fix: UX - Error logs in development from Empty state component (#466)
Browse files Browse the repository at this point in the history
Fixes #452 

When the empty state is rendered with a non-textual element (which it turns out all our current empty states are, because they have a `<button />` component as a call to action), this noisy error log was showing up in the `console`:

```
Warning: validateDOMNesting(...): <div> cannot appear as a descendant of <p>.
    at div
    at div
    at p
    at Typography (webpack-internal:///./node_modules/@material-ui/core/esm/Typography/Typography.js:166:28)
    at WithStyles (webpack-internal:///./node_modules/@material-ui/styles/esm/withStyles/withStyles.js:64:31)
    at div
    at StyledComponent (webpack-internal:///./node_modules/@material-ui/styles/esm/styled/styled.js:95:28)
    at EmptyState (webpack-internal:///./src/components/EmptyState/index.tsx:47:25)
...
    at ProjectsPage (webpack-internal:///./src/pages/projects/index.tsx:37:18)
    at Routes (webpack-internal:///./node_modules/react-router/index.js:275:5)
    at ThemeProvider (webpack-internal:///./node_modules/@material-ui/styles/esm/ThemeProvider/ThemeProvider.js:44:24)
    at UserProvider (webpack-internal:///./src/contexts/UserContext.tsx:100:55)
    at SWRConfig$1 (webpack-internal:///./node_modules/swr/dist/index.esm.js:501:23)
    at Router (webpack-internal:///./node_modules/react-router/index.js:209:15)
    at BrowserRouter (webpack-internal:///./node_modules/react-router-dom/index.js:118:5)
    at App
 ```

The issue was that the `description` prop could either be a `string` or an actual `React` component, but was always rendered as a child of a `<Typography />` component. The `<Typography>` component internally renders as a `<p>`, which is not valid to nest `<div>`s inside.

The fix is to not nest inside a `<Typography />` block, but an actual `<div />`.
  • Loading branch information
bryphe-coder committed Mar 17, 2022
1 parent 8b12e47 commit edd8345
Show file tree
Hide file tree
Showing 4 changed files with 48 additions and 8 deletions.
1 change: 1 addition & 0 deletions site/jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ module.exports = {
preset: "ts-jest",

roots: ["<rootDir>"],
setupFilesAfterEnv: ["./jest.setup.ts"],
transform: {
"^.+\\.tsx?$": "ts-jest",
},
Expand Down
22 changes: 22 additions & 0 deletions site/jest.setup.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// Helper utility to fail jest tests if a console.error is logged
// Pulled from this blog post:
// https://www.benmvp.com/blog/catch-warnings-jest-tests/

// For now, I limited this to just 'error' - but failing on warnings
// would be a nice next step! We may need to filter out some noise
// from material-ui though.
const CONSOLE_FAIL_TYPES = ["error" /* 'warn' */]

// Throw errors when a `console.error` or `console.warn` happens
// by overriding the functions
CONSOLE_FAIL_TYPES.forEach((logType: string) => {
// Suppressing the no-explicit-any to override certain console functions for testing
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const consoleAsAny = global.console as any
consoleAsAny[logType] = (message: string): void => {
throw new Error(`Failing due to console.${logType} while running test!\n\n${message}`)
}
})

// This is needed because we are compiling under `--isolatedModules`
export {}
21 changes: 21 additions & 0 deletions site/src/components/EmptyState/index.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,25 @@ describe("EmptyState", () => {
// Then
await screen.findByText("Hello, world")
})

it("renders description text", async () => {
// When
render(<EmptyState message="Hello, world" description="Friendly greeting" />)

// Then
await screen.findByText("Hello, world")
await screen.findByText("Friendly greeting")
})

it("renders description component", async () => {
// Given
const description = <button title="Click me" />

// When
render(<EmptyState message="Hello, world" description={description} />)

// Then
await screen.findByText("Hello, world")
await screen.findByRole("button")
})
})
12 changes: 4 additions & 8 deletions site/src/components/EmptyState/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import Typography from "@material-ui/core/Typography"

export interface EmptyStateProps {
/** Text Message to display, placed inside Typography component */
message: React.ReactNode
message: string
/** Longer optional description to display below the message */
description?: React.ReactNode
button?: ButtonProps
Expand All @@ -23,20 +23,14 @@ export interface EmptyStateProps {
export const EmptyState: React.FC<EmptyStateProps> = (props) => {
const { message, description, button, ...boxProps } = props
const styles = useStyles()

const descClassName = `${styles.description}`
const buttonClassName = `${styles.button} ${button && button.className ? button.className : ""}`

return (
<Box className={styles.root} {...boxProps}>
<Typography variant="h5" color="textSecondary" className={styles.header}>
{message}
</Typography>
{description && (
<Typography variant="body2" color="textSecondary" className={descClassName}>
{description}
</Typography>
)}
{description && <div className={styles.description}>{description}</div>}
{button && <Button variant="contained" color="primary" {...button} className={buttonClassName} />}
</Box>
)
Expand All @@ -59,6 +53,8 @@ const useStyles = makeStyles(
description: {
marginTop: theme.spacing(2),
marginBottom: theme.spacing(1),
color: theme.palette.text.secondary,
fontSize: theme.typography.body2.fontSize,
},
button: {
marginTop: theme.spacing(2),
Expand Down

0 comments on commit edd8345

Please sign in to comment.