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 2 commits
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
139 changes: 35 additions & 104 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.

4 changes: 3 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,12 +99,13 @@
"@github/relative-time-element": "^4.1.2",
"@lit-labs/react": "1.1.1",
"@primer/behaviors": "1.3.4",
"@primer/octicons-react": "18.3.0",
"@primer/octicons-react": "19.3.0",
"@primer/primitives": "7.11.11",
"@react-aria/ssr": "^3.1.0",
"@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
Loading