Skip to content

use react-is for checking valid element type, update primer/octicons-react #3437

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.

Sign up for GitHub

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

Merged
mattcosta7 merged 10 commits into from
Jun 22, 2023
Merged

use react-is for checking valid element type, update primer/octicons-react #3437

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7416720
use react-is for checking valid element type
mattcosta7 Jun 21, 2023
1923b6f
update primer
mattcosta7 Jun 21, 2023
02c2e02
update primer
mattcosta7 Jun 21, 2023
492bca9
update octicon
mattcosta7 Jun 21, 2023
8bcf91a
changeset
mattcosta7 Jun 21, 2023
340f0a5
loosen react-is types
mattcosta7 Jun 21, 2023
4955d42
Merge branch 'main' into use-isValidElementType-for-visuals
mattcosta7 Jun 21, 2023
4ebc57c
Merge branch 'main' into use-isValidElementType-for-visuals
mattcosta7 Jun 22, 2023
16880e8
update some types
mattcosta7 Jun 22, 2023
67f9ea3
test(vrt): update snapshots
broccolinisoup Jun 22, 2023
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
129 changes: 30 additions & 99 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@
"@styled-system/css": "^5.1.5",
"@styled-system/props": "^5.1.5",
"@styled-system/theme-get": "^5.1.2",
"@types/react-is": "18.2.1",
"@types/styled-components": "^5.1.11",
"@types/styled-system": "^5.1.12",
"@types/styled-system__css": "^5.0.16",
Expand All @@ -118,6 +119,7 @@
"lodash.isempty": "4.4.0",
"lodash.isobject": "3.0.2",
"react-intersection-observer": "9.4.3",
"react-is": "18.2.0",
Copy link
Contributor Author

@mattcosta7 mattcosta7 Jun 21, 2023

Choose a reason for hiding this comment

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

going to loosen this and types so they can be managed better in dotcom

"styled-system": "^5.1.5"
},
"devDependencies": {
Expand Down
13 changes: 9 additions & 4 deletions src/TextInput/TextInput.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import React, {MouseEventHandler, useCallback, useState} from 'react'
import {isValidElementType} from 'react-is'
import {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic'
import classnames from 'classnames'

Expand All @@ -24,11 +25,11 @@ export type TextInputNonPassthroughProps = {
/**
* A visual that renders inside the input before the typing area
*/
leadingVisual?: string | React.ComponentType<React.PropsWithChildren<{className?: string}>>
leadingVisual?: React.ElementType | React.ReactNode
Copy link
Contributor Author

@mattcosta7 mattcosta7 Jun 21, 2023

Choose a reason for hiding this comment

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

we don't pass props internally, so we don't need to actually care what the props the element can take are

Copy link
Member

@broccolinisoup broccolinisoup Jun 22, 2023

Choose a reason for hiding this comment

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

This might a be novice ts question but I assumed that only passing React.ReactNode will make ts happy (it doesn't!) because as far as I understand React.ReactNode includes React.ElementType as well? So I am curious why we need to explicitly type React.ElementType here as well 🤔

Copy link
Contributor Author

@mattcosta7 mattcosta7 Jun 22, 2023

Choose a reason for hiding this comment

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

ooo interesting. I didn't realize ReactNode included that, and i'm suprised it does (since ElementType isn't renderable as children directly). I think i'd prefer this since it's a bit more explicit that this could be a Component, but could also be renderable content?

/**
* A visual that renders inside the input after the typing area
*/
trailingVisual?: string | React.ComponentType<React.PropsWithChildren<{className?: string}>>
trailingVisual?: React.ElementType | React.ReactNode
/**
* A visual that renders inside the input after the typing area
*/
Expand Down Expand Up @@ -134,7 +135,7 @@ const TextInput = React.forwardRef<HTMLInputElement, TextInputProps>(
showLoadingIndicator={showLeadingLoadingIndicator}
hasLoadingIndicator={typeof loading === 'boolean'}
>
{typeof LeadingVisual === 'function' ? <LeadingVisual /> : LeadingVisual}
{typeof LeadingVisual !== 'string' && isValidElementType(LeadingVisual) ? <LeadingVisual /> : LeadingVisual}
</TextInputInnerVisualSlot>
<UnstyledTextInput
ref={inputRef}
Expand All @@ -150,7 +151,11 @@ const TextInput = React.forwardRef<HTMLInputElement, TextInputProps>(
showLoadingIndicator={showTrailingLoadingIndicator}
hasLoadingIndicator={typeof loading === 'boolean'}
>
{typeof TrailingVisual === 'function' ? <TrailingVisual /> : TrailingVisual}
{typeof TrailingVisual !== 'string' && isValidElementType(TrailingVisual) ? (
<TrailingVisual />
) : (
TrailingVisual
)}
</TextInputInnerVisualSlot>
{trailingAction}
</TextInputWrapper>
Expand Down
5 changes: 3 additions & 2 deletions src/TextInputWithTokens/TextInputWithTokens.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {FocusKeys} from '@primer/behaviors'
import {isFocusable} from '@primer/behaviors/utils'
import {omit} from '@styled-system/props'
import React, {FocusEventHandler, KeyboardEventHandler, MouseEventHandler, RefObject, useRef, useState} from 'react'
import {isValidElementType} from 'react-is'
import Box from '../Box'
import {useRefObjectAsForwardedRef} from '../hooks/useRefObjectAsForwardedRef'
import {useFocusZone} from '../hooks/useFocusZone'
Expand Down Expand Up @@ -296,7 +297,7 @@ function TextInputWithTokensInnerComponent<TokenComponentType extends AnyReactCo
visualPosition="leading"
showLoadingIndicator={showLeadingLoadingIndicator}
>
{typeof LeadingVisual === 'function' ? <LeadingVisual /> : LeadingVisual}
{typeof LeadingVisual !== 'string' && isValidElementType(LeadingVisual) ? <LeadingVisual /> : LeadingVisual}
Copy link
Contributor Author

@mattcosta7 mattcosta7 Jun 21, 2023

Choose a reason for hiding this comment

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

we have to check for string explicitly here, because things like 'div' is a valid element type, but we don't want to try and render an empty div, but instead just pass through that text.

We may want to consider whether accepting raw element types makes sense longterm (I don't think there's a good reason to accept a raw Component instead of just React.ReactNode).

<Component leadingVisual={SearchIcon} /> // imo this shouldn't be allowed

<Component leadingVisual={<SearchIcon />} /> // imo this should be the way

but that might be a later breaking change at this point?

Copy link
Member

@joshblack joshblack Jun 21, 2023

Choose a reason for hiding this comment

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

@mattcosta7 I think you're dead-on and I bet we could look to introduce this behavior in v36 and deprecate the raw element type and remove support for it in v37

Copy link
Member

@broccolinisoup broccolinisoup Jun 22, 2023

Choose a reason for hiding this comment

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

Oh great point! Thanks for the note as well, that was one of my questions until I came to that point!

➕ 1 for using ReactNodes instead of raw components

</TextInputInnerVisualSlot>
<Box
ref={containerRef as RefObject<HTMLDivElement>}
Expand Down Expand Up @@ -362,7 +363,7 @@ function TextInputWithTokensInnerComponent<TokenComponentType extends AnyReactCo
visualPosition="trailing"
showLoadingIndicator={showTrailingLoadingIndicator}
>
{typeof TrailingVisual === 'function' ? <TrailingVisual /> : TrailingVisual}
{typeof TrailingVisual !== 'string' && isValidElementType(TrailingVisual) ? <TrailingVisual /> : TrailingVisual}
</TextInputInnerVisualSlot>
</TextInputWrapper>
)
Expand Down
Loading