From 24b1ebbc21ca2eaa92bd6b5b8c15ab0e7577f5a0 Mon Sep 17 00:00:00 2001 From: Pavithra Kodmad Date: Wed, 10 Nov 2021 12:23:27 +1100 Subject: [PATCH 1/4] [WIP]Use Isomorphic effect (#1583) * Use Isomorphic effect * Create brown-squids-unite.md * Fix eslint rules * Make it typescript * Add back the eslint disables * Fix up the tests and make the method more robust * Fix lint Co-authored-by: Pavithra Kodmad --- .changeset/brown-squids-unite.md | 5 +++++ src/Overlay.tsx | 3 ++- src/Portal/Portal.tsx | 3 ++- src/hooks/useAnchoredPosition.ts | 3 ++- src/hooks/useCombinedRefs.ts | 6 +++--- src/hooks/useResizeObserver.ts | 4 ++-- src/utils/useIsomorphicLayoutEffect.ts | 10 ++++++++++ 7 files changed, 26 insertions(+), 8 deletions(-) create mode 100644 .changeset/brown-squids-unite.md create mode 100644 src/utils/useIsomorphicLayoutEffect.ts diff --git a/.changeset/brown-squids-unite.md b/.changeset/brown-squids-unite.md new file mode 100644 index 00000000000..660049b1b02 --- /dev/null +++ b/.changeset/brown-squids-unite.md @@ -0,0 +1,5 @@ +--- +"@primer/components": patch +--- + +Add a utility to provide useIsoMorphicEffect function and use that instead of useLayoutEffect everywhere diff --git a/src/Overlay.tsx b/src/Overlay.tsx index 534ccf13b1f..c32cc2a0bf6 100644 --- a/src/Overlay.tsx +++ b/src/Overlay.tsx @@ -1,7 +1,8 @@ import styled from 'styled-components' -import React, {ReactElement, useEffect, useLayoutEffect, useRef} from 'react' +import React, {ReactElement, useEffect, useRef} from 'react' import {get, COMMON, SystemPositionProps, SystemCommonProps} from './constants' import {ComponentProps} from './utils/types' +import useLayoutEffect from './utils/useIsomorphicLayoutEffect' import {useOverlay, TouchOrMouseEvent} from './hooks' import Portal from './Portal' import sx, {SxProp} from './sx' diff --git a/src/Portal/Portal.tsx b/src/Portal/Portal.tsx index a2eaf32fecc..bc5445e9143 100644 --- a/src/Portal/Portal.tsx +++ b/src/Portal/Portal.tsx @@ -1,5 +1,6 @@ import React from 'react' import {createPortal} from 'react-dom' +import useLayoutEffect from '../utils/useIsomorphicLayoutEffect' const PRIMER_PORTAL_ROOT_ID = '__primerPortalRoot__' const DEFAULT_PORTAL_CONTAINER_NAME = '__default__' @@ -69,7 +70,7 @@ export const Portal: React.FC = ({children, onMount, containerName: hostElement.style.zIndex = '1' const elementRef = React.useRef(hostElement) - React.useLayoutEffect(() => { + useLayoutEffect(() => { let containerName = _containerName if (containerName === undefined) { containerName = DEFAULT_PORTAL_CONTAINER_NAME diff --git a/src/hooks/useAnchoredPosition.ts b/src/hooks/useAnchoredPosition.ts index 5ff674833a3..4134916316c 100644 --- a/src/hooks/useAnchoredPosition.ts +++ b/src/hooks/useAnchoredPosition.ts @@ -2,6 +2,7 @@ import React from 'react' import {PositionSettings, getAnchoredPosition, AnchorPosition} from '../behaviors/anchoredPosition' import {useProvidedRefOrCreate} from './useProvidedRefOrCreate' import {useResizeObserver} from './useResizeObserver' +import useLayoutEffect from '../utils/useIsomorphicLayoutEffect' export interface AnchoredPositionHookSettings extends Partial { floatingElementRef?: React.RefObject @@ -41,7 +42,7 @@ export function useAnchoredPosition( [floatingElementRef, anchorElementRef, ...dependencies] ) - React.useLayoutEffect(updatePosition, [updatePosition]) + useLayoutEffect(updatePosition, [updatePosition]) useResizeObserver(updatePosition) diff --git a/src/hooks/useCombinedRefs.ts b/src/hooks/useCombinedRefs.ts index 0eedc6beb5e..8eb341ea1a6 100644 --- a/src/hooks/useCombinedRefs.ts +++ b/src/hooks/useCombinedRefs.ts @@ -1,4 +1,5 @@ -import React, {ForwardedRef, useRef} from 'react' +import {ForwardedRef, useRef} from 'react' +import useLayoutEffect from '../utils/useIsomorphicLayoutEffect' /** * Creates a ref by combining multiple constituent refs. The ref returned by this hook @@ -11,7 +12,7 @@ import React, {ForwardedRef, useRef} from 'react' export function useCombinedRefs(...refs: (ForwardedRef | null | undefined)[]) { const combinedRef = useRef(null) - React.useLayoutEffect(() => { + useLayoutEffect(() => { function setRefs(current: T | null = null) { for (const ref of refs) { if (!ref) { @@ -32,7 +33,6 @@ export function useCombinedRefs(...refs: (ForwardedRef | null | undefined) // eslint-disable-next-line react-hooks/exhaustive-deps setRefs(combinedRef.current) } - // eslint-disable-next-line react-hooks/exhaustive-deps }, [...refs, combinedRef.current]) diff --git a/src/hooks/useResizeObserver.ts b/src/hooks/useResizeObserver.ts index c18d9f83453..fee96fcb1c4 100644 --- a/src/hooks/useResizeObserver.ts +++ b/src/hooks/useResizeObserver.ts @@ -1,7 +1,7 @@ -import React from 'react' +import useLayoutEffect from '../utils/useIsomorphicLayoutEffect' export function useResizeObserver(callback: () => void) { - React.useLayoutEffect(() => { + useLayoutEffect(() => { const observer = new window.ResizeObserver(() => callback()) observer.observe(document.documentElement) return () => { diff --git a/src/utils/useIsomorphicLayoutEffect.ts b/src/utils/useIsomorphicLayoutEffect.ts new file mode 100644 index 00000000000..6213d5af4e0 --- /dev/null +++ b/src/utils/useIsomorphicLayoutEffect.ts @@ -0,0 +1,10 @@ +import {useEffect, useLayoutEffect} from 'react' + +const useIsomorphicLayoutEffect = + typeof window !== 'undefined' && + typeof window.document !== 'undefined' && + typeof window.document.createElement !== 'undefined' + ? useLayoutEffect + : useEffect + +export default useIsomorphicLayoutEffect From f3c23308fcbac21b6b95336e37a77b54779b0a10 Mon Sep 17 00:00:00 2001 From: Jonathan Fuchs Date: Wed, 10 Nov 2021 09:43:10 -0800 Subject: [PATCH 2/4] Set our script to run with max-warnings=0 (#1587) --- .../@primer/gatsby-theme-doctocat/components/hero.js | 4 +--- .../components/live-preview-wrapper.js | 2 +- migrating.md | 2 +- package.json | 2 +- src/__tests__/TextInputWithTokens.test.tsx | 10 ---------- 5 files changed, 4 insertions(+), 16 deletions(-) diff --git a/docs/src/@primer/gatsby-theme-doctocat/components/hero.js b/docs/src/@primer/gatsby-theme-doctocat/components/hero.js index d9bbf459ed7..ea0c0737cdf 100644 --- a/docs/src/@primer/gatsby-theme-doctocat/components/hero.js +++ b/docs/src/@primer/gatsby-theme-doctocat/components/hero.js @@ -9,9 +9,7 @@ export default function Hero() { - - Primer React - + Primer React v{version} diff --git a/docs/src/@primer/gatsby-theme-doctocat/components/live-preview-wrapper.js b/docs/src/@primer/gatsby-theme-doctocat/components/live-preview-wrapper.js index c863858413d..98b814ef9ef 100644 --- a/docs/src/@primer/gatsby-theme-doctocat/components/live-preview-wrapper.js +++ b/docs/src/@primer/gatsby-theme-doctocat/components/live-preview-wrapper.js @@ -8,7 +8,7 @@ function ThemeSwitcher() { return ( ( - + {children} )} diff --git a/migrating.md b/migrating.md index 98e7839075f..ef9ba0ba040 100644 --- a/migrating.md +++ b/migrating.md @@ -110,7 +110,7 @@ There are two ways to change the theme of @primer/components components: export default () => ( - + Hello, world! diff --git a/package.json b/package.json index e7eea4fa15a..bdbf226beb2 100644 --- a/package.json +++ b/package.json @@ -15,7 +15,7 @@ "start": "concurrently npm:start:*", "start:docs": "cd docs && npm run develop", "start:storybook": "start-storybook -p 6006", - "lint": "eslint '**/*.{js,ts,tsx,md,mdx}'", + "lint": "eslint '**/*.{js,ts,tsx,md,mdx}' --max-warnings=0", "lint:fix": "npm run lint -- --fix", "test": "jest", "test:update": "npm run test -- --updateSnapshot", diff --git a/src/__tests__/TextInputWithTokens.test.tsx b/src/__tests__/TextInputWithTokens.test.tsx index f3f43886547..17d032e3fd6 100644 --- a/src/__tests__/TextInputWithTokens.test.tsx +++ b/src/__tests__/TextInputWithTokens.test.tsx @@ -29,16 +29,6 @@ const LabelledTextInputWithTokens: React.FC = ({onToke ) -// describe('axe test', () => { -// it('should have no axe violations', async () => { -// const onRemoveMock = jest.fn() -// const {container} = HTMLRender() -// const results = await axe(container) -// expect(results).toHaveNoViolations() -// cleanup() -// }) -// }) - jest.useFakeTimers() describe('TextInputWithTokens', () => { From 2a61b8d0e21f86373d9ff0f5d9add83f9aee2d7f Mon Sep 17 00:00:00 2001 From: Jonathan Fuchs Date: Wed, 10 Nov 2021 13:34:50 -0800 Subject: [PATCH 3/4] Fix an AnchoredOverlay test to clean up snapshots (#1586) --- src/__tests__/AnchoredOverlay.test.tsx | 4 +- .../AnchoredOverlay.test.tsx.snap | 170 ++++-------------- 2 files changed, 37 insertions(+), 137 deletions(-) diff --git a/src/__tests__/AnchoredOverlay.test.tsx b/src/__tests__/AnchoredOverlay.test.tsx index e6445072609..29f2ff6a377 100644 --- a/src/__tests__/AnchoredOverlay.test.tsx +++ b/src/__tests__/AnchoredOverlay.test.tsx @@ -144,7 +144,7 @@ describe('AnchoredOverlay', () => { }) it('should render consistently when open', () => { - const anchoredOverlay = HTMLRender() - expect(anchoredOverlay).toMatchSnapshot() + const {container} = HTMLRender() + expect(container).toMatchSnapshot() }) }) diff --git a/src/__tests__/__snapshots__/AnchoredOverlay.test.tsx.snap b/src/__tests__/__snapshots__/AnchoredOverlay.test.tsx.snap index 285f69f5b8b..12c019b8f61 100644 --- a/src/__tests__/__snapshots__/AnchoredOverlay.test.tsx.snap +++ b/src/__tests__/__snapshots__/AnchoredOverlay.test.tsx.snap @@ -94,9 +94,7 @@ exports[`AnchoredOverlay renders consistently 1`] = ` `; exports[`AnchoredOverlay should render consistently when open 1`] = ` -Object { - "asFragment": [Function], - "baseElement": .c0 { +.c0 { font-family: -apple-system,BlinkMacSystemFont,"Segoe UI",Helvetica,Arial,sans-serif,"Apple Color Emoji","Segoe UI Emoji"; line-height: 1.5; color: #24292f; @@ -187,146 +185,48 @@ Object { outline: none; } - -
-
- -
-
-
- -
-
-
-
-
- , - "container":
+
+
+
-
-
- -
+ Focusable Child +
-
, - "debug": [Function], - "findAllByAltText": [Function], - "findAllByDisplayValue": [Function], - "findAllByLabelText": [Function], - "findAllByPlaceholderText": [Function], - "findAllByRole": [Function], - "findAllByTestId": [Function], - "findAllByText": [Function], - "findAllByTitle": [Function], - "findByAltText": [Function], - "findByDisplayValue": [Function], - "findByLabelText": [Function], - "findByPlaceholderText": [Function], - "findByRole": [Function], - "findByTestId": [Function], - "findByText": [Function], - "findByTitle": [Function], - "getAllByAltText": [Function], - "getAllByDisplayValue": [Function], - "getAllByLabelText": [Function], - "getAllByPlaceholderText": [Function], - "getAllByRole": [Function], - "getAllByTestId": [Function], - "getAllByText": [Function], - "getAllByTitle": [Function], - "getByAltText": [Function], - "getByDisplayValue": [Function], - "getByLabelText": [Function], - "getByPlaceholderText": [Function], - "getByRole": [Function], - "getByTestId": [Function], - "getByText": [Function], - "getByTitle": [Function], - "queryAllByAltText": [Function], - "queryAllByDisplayValue": [Function], - "queryAllByLabelText": [Function], - "queryAllByPlaceholderText": [Function], - "queryAllByRole": [Function], - "queryAllByTestId": [Function], - "queryAllByText": [Function], - "queryAllByTitle": [Function], - "queryByAltText": [Function], - "queryByDisplayValue": [Function], - "queryByLabelText": [Function], - "queryByPlaceholderText": [Function], - "queryByRole": [Function], - "queryByTestId": [Function], - "queryByText": [Function], - "queryByTitle": [Function], - "rerender": [Function], - "unmount": [Function], -} +
+
`; From 8d3d491fa3c9437b3a8f310e233b7009cae13095 Mon Sep 17 00:00:00 2001 From: Rez Date: Wed, 10 Nov 2021 23:03:07 +0000 Subject: [PATCH 4/4] Fix: TextInput styling regression with icon and block prop usage (#1592) --- .changeset/hip-ravens-sell.md | 5 ++ src/_TextInputWrapper.tsx | 7 ++ src/stories/TextInput.stories.tsx | 113 ++++++++++++++++++++++++++++++ 3 files changed, 125 insertions(+) create mode 100644 .changeset/hip-ravens-sell.md create mode 100644 src/stories/TextInput.stories.tsx diff --git a/.changeset/hip-ravens-sell.md b/.changeset/hip-ravens-sell.md new file mode 100644 index 00000000000..b5ff9010577 --- /dev/null +++ b/.changeset/hip-ravens-sell.md @@ -0,0 +1,5 @@ +--- +'@primer/components': patch +--- + +Fixes a styling bug in TextInput components while using its `icon` and `block` props together diff --git a/src/_TextInputWrapper.tsx b/src/_TextInputWrapper.tsx index 83822045cb5..fbc4da61e13 100644 --- a/src/_TextInputWrapper.tsx +++ b/src/_TextInputWrapper.tsx @@ -91,6 +91,13 @@ const TextInputWrapper = styled.span` display: block; width: 100%; `} + + ${props => + props.block && + props.hasIcon && + css` + display: flex; + `} // Ensures inputs don't zoom on mobile but are body-font size on desktop @media (min-width: ${get('breakpoints.1')}) { diff --git a/src/stories/TextInput.stories.tsx b/src/stories/TextInput.stories.tsx new file mode 100644 index 00000000000..0d8c9fb8fdc --- /dev/null +++ b/src/stories/TextInput.stories.tsx @@ -0,0 +1,113 @@ +import React, {useState, ReactNode} from 'react' +import {Meta} from '@storybook/react' + +import {BaseStyles, Box, ThemeProvider, Text} from '..' +import TextInput, {TextInputProps} from '../TextInput' +import {CheckIcon} from '@primer/octicons-react' + +export default { + title: 'Forms/Text Input', + component: TextInput, + decorators: [ + Story => { + return ( + + + {Story()} + + + ) + } + ], + argTypes: { + sx: { + table: { + disable: true + } + }, + block: { + name: 'Block', + defaultValue: false, + control: { + type: 'boolean' + } + }, + disabled: { + name: 'Disabled', + defaultValue: false, + control: { + type: 'boolean' + } + }, + variant: { + name: 'Variants', + options: ['small', 'medium', 'large'], + control: {type: 'radio'} + } + } +} as Meta + +const Label = ({htmlFor, children}: {htmlFor: string; children: ReactNode}) => ( + + {children} + +) + +export const Default = (args: TextInputProps) => { + const [value, setValue] = useState('') + + const handleChange = (event: React.ChangeEvent) => { + setValue(event.target.value) + } + + const inputId = 'basic-text-input' + + return ( +
+
+
+ +
+
+ +
+
+
+ ) +} + +export const WithLeadingIcon = (args: TextInputProps) => { + const [value, setValue] = useState('') + + const handleChange = (event: React.ChangeEvent) => { + setValue(event.target.value) + } + + const inputId = 'basic-text-input-with-leading-icon' + + return ( +
+ +
+ + + ) +} + +export const Password = (args: TextInputProps) => { + const [value, setValue] = useState('') + + const handleChange = (event: React.ChangeEvent) => { + setValue(event.target.value) + } + + const inputId = 'basic-text-input-as-password' + + return ( +
+ +
+ + + ) +}