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 all commits
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
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
34 changes: 5 additions & 29 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 @@ -39,9 +14,10 @@ const NavTabs = ({ tabs }: { tabs: NavItem[] }) => {
<Tabs value={activeTab} variant="scrollable" allowScrollButtonsMobile className={css.tabs}>
{tabs.map((tab, idx) => (
<Tab
component={NextLinkComposed}
key={tab.href}
href={{ pathname: tab.href, query }}
component={NextLink}
tabIndex={0}
className={css.tab}
label={
<Typography
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
19 changes: 1 addition & 18 deletions ...nts/tx/confirmation-views/BatchTransactions/__snapshots__/BatchTransactions.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,24 +194,7 @@ exports[`BatchTransactions should render a list of batch transactions 1`] = `
</span>
<div
class="MuiBox-root css-yjghm1"
>
<button
aria-label=""
class="MuiButtonBase-root MuiIconButton-root MuiIconButton-sizeSmall css-1vbp2rr-MuiButtonBase-root-MuiIconButton-root"
data-mui-internal-clone-element="true"
data-testid="explorer-btn"
rel="noreferrer"
tabindex="0"
target="_blank"
type="button"
>
<mock-icon
aria-hidden=""
class="MuiSvgIcon-root MuiSvgIcon-fontSizeSmall css-tqxw8e-MuiSvgIcon-root"
focusable="false"
/>
</button>
</div>
/>
<button
class="MuiButtonBase-root MuiIconButton-root MuiIconButton-edgeEnd MuiIconButton-sizeSmall css-1bg1tw1-MuiButtonBase-root-MuiIconButton-root"
tabindex="0"
Expand Down
57 changes: 3 additions & 54 deletions ...omponents/tx/confirmation-views/SettingsChange/__snapshots__/SettingsChange.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,24 +80,7 @@ exports[`SettingsChange should display the SettingsChange component with newOwne
</span>
<div
class="MuiBox-root css-yjghm1"
>
<button
aria-label=""
class="MuiButtonBase-root MuiIconButton-root MuiIconButton-sizeSmall css-1vbp2rr-MuiButtonBase-root-MuiIconButton-root"
data-mui-internal-clone-element="true"
data-testid="explorer-btn"
rel="noreferrer"
tabindex="0"
target="_blank"
type="button"
>
<mock-icon
aria-hidden=""
class="MuiSvgIcon-root MuiSvgIcon-fontSizeSmall css-tqxw8e-MuiSvgIcon-root"
focusable="false"
/>
</button>
</div>
/>
</div>
</div>
</div>
Expand Down Expand Up @@ -180,24 +163,7 @@ exports[`SettingsChange should display the SettingsChange component with newOwne
</span>
<div
class="MuiBox-root css-yjghm1"
>
<button
aria-label=""
class="MuiButtonBase-root MuiIconButton-root MuiIconButton-sizeSmall css-1vbp2rr-MuiButtonBase-root-MuiIconButton-root"
data-mui-internal-clone-element="true"
data-testid="explorer-btn"
rel="noreferrer"
tabindex="0"
target="_blank"
type="button"
>
<mock-icon
aria-hidden=""
class="MuiSvgIcon-root MuiSvgIcon-fontSizeSmall css-tqxw8e-MuiSvgIcon-root"
focusable="false"
/>
</button>
Comment on lines -184 to -199
Copy link
Member Author

Choose a reason for hiding this comment

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

This is not rendered anymore in the snapshot because we are not mocking the chain and with my change, the explorer link is not rendered anymore if there is no href

</div>
/>
</div>
</div>
</div>
Expand Down Expand Up @@ -288,24 +254,7 @@ exports[`SettingsChange should display the SettingsChange component with owner d
</span>
<div
class="MuiBox-root css-yjghm1"
>
<button
aria-label=""
class="MuiButtonBase-root MuiIconButton-root MuiIconButton-sizeSmall css-1vbp2rr-MuiButtonBase-root-MuiIconButton-root"
data-mui-internal-clone-element="true"
data-testid="explorer-btn"
rel="noreferrer"
tabindex="0"
target="_blank"
type="button"
>
<mock-icon
aria-hidden=""
class="MuiSvgIcon-root MuiSvgIcon-fontSizeSmall css-tqxw8e-MuiSvgIcon-root"
focusable="false"
/>
</button>
</div>
/>
</div>
</div>
</div>
Expand Down
Loading
Loading