Skip to content
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

fix: React props errors #4692

Merged
merged 3 commits into from
Dec 19, 2024
Merged

fix: React props errors #4692

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ae402be
fix: React props errors
usame-algan Dec 19, 2024
b786db8
chore: Update snapshots
usame-algan Dec 19, 2024
70a9c29
fix: Use NextLink inside Tab component instead of wrapping
usame-algan Dec 19, 2024
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 .yarnrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
# yarn lockfile v1


yarn-path ".yarn/releases/yarn-1.22.22.cjs"
katspaugh marked this conversation as resolved.
Show resolved Hide resolved
13 changes: 8 additions & 5 deletions src/components/common/AddressBookInput/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,14 @@ const AddressBookInput = ({ name, canAdd, ...props }: AddressInputProps & { canA
elevation: 2,
},
}}
renderOption={(props, option) => (
<Typography data-testid="address-item" component="li" variant="body2" {...props}>
<EthHashInfo address={option.label} name={option.name} shortAddress={false} copyAddress={false} />
</Typography>
)}
renderOption={(props, option) => {
const { key, ...rest } = props
return (
<Typography data-testid="address-item" component="li" variant="body2" {...rest} key={key}>
<EthHashInfo address={option.label} name={option.name} shortAddress={false} copyAddress={false} />
</Typography>
)
}}
renderInput={(params) => (
<AddressInput
data-testid="address-item"
Copy link

Choose a reason for hiding this comment

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

The use of destructuring to extract key from props is unnecessary as key is not needed for the child components. Remove destructuring and pass props directly: return (<Typography {...props} data-testid="address-item" ...> This keeps the code concise and aligned with best practices.

Expand Down
4 changes: 3 additions & 1 deletion src/components/common/ExplorerButton/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ const ExplorerButton = ({
className,
onClick,
isCompact = true,
}: ExplorerButtonProps): ReactElement => {
}: ExplorerButtonProps): ReactElement | null => {
if (!href) return null

return isCompact ? (
<Tooltip title={title} placement="top">
<IconButton
Copy link

Choose a reason for hiding this comment

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

Ensure href is included in the ExplorerButtonProps type definition. This prevents type errors and makes the purpose of the check clear. If href is not a required prop, consider if null is the appropriate fallback behavior or if a default action is preferable.

Expand Down
64 changes: 20 additions & 44 deletions src/components/common/NavTabs/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,35 +1,10 @@
import React, { forwardRef } from 'react'
import NextLink, { type LinkProps as NextLinkProps } from 'next/link'
import { Tab, Tabs, Typography, type TabProps } from '@mui/material'
import React from 'react'
import NextLink from 'next/link'
import { Tab, Tabs, Typography } from '@mui/material'
import { useRouter } from 'next/router'
import type { NavItem } from '@/components/sidebar/SidebarNavigation/config'
import css from './styles.module.css'

type Props = TabProps & NextLinkProps

// This is needed in order for the tabs to work properly with Next Link e.g. tabbing with the keyboard
// Based on https://github.com/mui/material-ui/blob/master/examples/nextjs-with-typescript/src/Link.tsx
const NextLinkComposed = forwardRef<HTMLAnchorElement, Props>(function NextComposedLink(props: Props, ref) {
const { href, as, replace, scroll, shallow, prefetch, legacyBehavior = true, locale, ...other } = props

return (
<NextLink
href={href}
prefetch={prefetch}
as={as}
replace={replace}
scroll={scroll}
shallow={shallow}
passHref
locale={locale}
legacyBehavior={legacyBehavior}
>
{/* @ts-ignore */}
<a ref={ref} {...other} />
</NextLink>
)
})

const NavTabs = ({ tabs }: { tabs: NavItem[] }) => {
const router = useRouter()
const activeTab = Math.max(0, tabs.map((tab) => tab.href).indexOf(router.pathname))
Expand All @@ -38,22 +13,23 @@ const NavTabs = ({ tabs }: { tabs: NavItem[] }) => {
return (
<Tabs value={activeTab} variant="scrollable" allowScrollButtonsMobile className={css.tabs}>
{tabs.map((tab, idx) => (
<Tab
component={NextLinkComposed}
key={tab.href}
href={{ pathname: tab.href, query }}
className={css.tab}
label={
<Typography
variant="body2"
fontWeight={700}
color={activeTab === idx ? 'primary' : 'primary.light'}
className={css.label}
>
{tab.label}
</Typography>
}
/>
<NextLink key={tab.href} href={{ pathname: tab.href, query }} passHref legacyBehavior>
<Tab
component="a"
tabIndex={0}
className={css.tab}
label={
<Typography
variant="body2"
fontWeight={700}
color={activeTab === idx ? 'primary' : 'primary.light'}
className={css.label}
>
{tab.label}
</Typography>
}
/>
</NextLink>
))}
</Tabs>
)
Expand Down
18 changes: 11 additions & 7 deletions src/components/common/NetworkSelector/NetworkMultiSelector.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import useChains, { useCurrentChain } from '@/hooks/useChains'
import useSafeAddress from '@/hooks/useSafeAddress'
import { useCallback, useEffect, type ReactElement } from 'react'
import { Checkbox, Autocomplete, TextField, Chip } from '@mui/material'
import { Checkbox, Autocomplete, TextField, Chip, Box } from '@mui/material'
import type { ChainInfo } from '@safe-global/safe-gateway-typescript-sdk'
import ChainIndicator from '../ChainIndicator'
import css from './styles.module.css'
Expand Down Expand Up @@ -138,12 +138,16 @@ const NetworkMultiSelector = ({
></Chip>
))
}
renderOption={(props, chain, { selected }) => (
<li {...props} key={chain.chainId}>
<Checkbox data-testid="network-checkbox" size="small" checked={selected} />
<ChainIndicator chainId={chain.chainId} inline />
</li>
)}
renderOption={(props, chain, { selected }) => {
const { key, ...rest } = props

return (
<Box component="li" key={key} {...rest}>
<Checkbox data-testid="network-checkbox" size="small" checked={selected} />
<ChainIndicator chainId={chain.chainId} inline />
</Box>
)
}}
getOptionLabel={(option) => option.chainName}
getOptionDisabled={isOptionDisabled}
renderInput={(params) => (
Copy link

Choose a reason for hiding this comment

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

Consider extracting the renderOption function into a separate component to improve readability and reusability. Adopting this approach not only aligns with the DRY principle but also enhances code clarity and maintainability.

Expand Down
6 changes: 4 additions & 2 deletions src/components/tx-flow/common/TxNonce/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,11 +217,13 @@ const TxNonceForm = ({ nonce, recommendedNonce }: { nonce: string; recommendedNo
const isRecommendedNonce = option === recommendedNonce
const isInitialPreviousNonce = option === previousNonces[0]

const { key, ...rest } = props

return (
<div key={option}>
<div key={key}>
{isRecommendedNonce && <NonceFormHeader>Recommended nonce</NonceFormHeader>}
{isInitialPreviousNonce && <NonceFormHeader sx={{ pt: 3 }}>Replace existing</NonceFormHeader>}
<NonceFormOption key={option} menuItemProps={props} nonce={option} />
<NonceFormOption menuItemProps={rest} nonce={option} />
</div>
)
}}
Copy link

Choose a reason for hiding this comment

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

Consider renaming key to something more descriptive like optionKey for clarity, given its significance in React's reconciliation. Check the safety of destructuring props without knowing its complete structure; ensure it doesn't remove required properties.

Expand Down
Loading