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: signer vs plural signers #4675

Merged
merged 1 commit into from
Dec 17, 2024
Merged

Fix: signer vs plural signers #4675

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion src/components/settings/RequiredConfirmations/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { ChangeThresholdFlow } from '@/components/tx-flow/flows'
import CheckWallet from '@/components/common/CheckWallet'
import { useContext } from 'react'
import { TxModalContext } from '@/components/tx-flow'
import { maybePlural } from '@/utils/formatters'

export const RequiredConfirmation = ({ threshold, owners }: { threshold: number; owners: number }) => {
const { setTxFlow } = useContext(TxModalContext)
Expand Down Expand Up @@ -43,7 +44,7 @@ export const RequiredConfirmation = ({ threshold, owners }: { threshold: number;
pr: 2,
}}
>
<b>{threshold}</b> out of <b>{owners}</b> signers.
<b>{threshold}</b> out of <b>{owners}</b> signer{maybePlural(owners)}.
</Typography>

{owners > 1 && (
Copy link

Choose a reason for hiding this comment

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

Consider moving maybePlural logic out of template literal for clarity. Refactor to use a single function call within the component to determine singular or plural form, to enhance readability and maintainability.

Expand Down
3 changes: 2 additions & 1 deletion src/components/tx-flow/flows/RemoveOwner/ReviewRemoveOwner.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import EthHashInfo from '@/components/common/EthHashInfo'

import commonCss from '@/components/tx-flow/common/styles.module.css'
import { ChangeSignerSetupWarning } from '@/features/multichain/components/SignerSetupWarning/ChangeSignerSetupWarning'
import { maybePlural } from '@/utils/formatters'

export const ReviewRemoveOwner = ({ params }: { params: RemoveOwnerFlowProps }): ReactElement => {
const addressBook = useAddressBook()
Expand Down Expand Up @@ -55,7 +56,7 @@ export const ReviewRemoveOwner = ({ params }: { params: RemoveOwnerFlowProps }):
Any transaction requires the confirmation of:
</Typography>
<Typography>
<b>{threshold}</b> out of <b>{newOwnerLength}</b> signers
<b>{threshold}</b> out of <b>{newOwnerLength}</b> signer{maybePlural(newOwnerLength)}
</Typography>
</Box>
<Divider className={commonCss.nestedDivider} />
Copy link

Choose a reason for hiding this comment

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

  1. Consider renaming maybePlural to something more descriptive like pluralizeIfNeeded to enhance readability.
  2. Inline logic for pluralization could be moved to a utility file if used across multiple components, adhering to DRY principles.

Expand Down
2 changes: 1 addition & 1 deletion ...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 @@ -331,7 +331,7 @@ exports[`SettingsChange should display the SettingsChange component with owner d

<b>
1
signers
signer
</b>
</p>
</div>
Copy link

Choose a reason for hiding this comment

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

Ensure the conditional for singular/plural form is accurate. Changing "signers" to "signer" might be incorrect if it's intended for a plural context. Verify the logic, handling both singular and plural cases to avoid linguistic errors.

Expand Down
6 changes: 5 additions & 1 deletion src/components/tx/confirmation-views/SettingsChange/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { SettingsInfoType, type SettingsChange } from '@safe-global/safe-gateway
import { ChangeSignerSetupWarning } from '@/features/multichain/components/SignerSetupWarning/ChangeSignerSetupWarning'
import { useContext } from 'react'
import { SettingsChangeContext } from '@/components/tx-flow/flows/AddOwner/context'
import { maybePlural } from '@/utils/formatters'

export interface SettingsChangeProps extends NarrowConfirmationViewProps {
txInfo: SettingsChange
Expand All @@ -22,6 +23,7 @@ const SettingsChange: React.FC<SettingsChangeProps> = ({ txInfo: { settingsInfo

const shouldShowChangeSigner = 'owner' in settingsInfo || 'newOwner' in params
const hasNewOwner = 'newOwner' in params
const newSignersLength = safe.owners.length + ('removedOwner' in settingsInfo ? 0 : 1)

return (
<>
Expand Down Expand Up @@ -54,7 +56,9 @@ const SettingsChange: React.FC<SettingsChangeProps> = ({ txInfo: { settingsInfo
<Typography variant="body2">Any transaction requires the confirmation of:</Typography>
<Typography>
<b>{settingsInfo.threshold}</b> out of{' '}
<b>{safe.owners.length + ('removedOwner' in settingsInfo ? 0 : 1)} signers</b>
<b>
{newSignersLength} signer{maybePlural(newSignersLength)}
</b>
</Typography>
</Box>
</>
Copy link

Choose a reason for hiding this comment

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

 const newSignersLength = safe.owners.length + ('removedOwner' in settingsInfo ? 0 : 1);

Consider encapsulating the logic to determine the number of signers inside a function for clarity and reuse. This will simplify the component and improve code readability.

Expand Down
2 changes: 1 addition & 1 deletion src/components/tx/confirmation-views/__snapshots__/ConfirmationView.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ exports[`ConfirmationView should display a confirmation screen for a SETTINGS_CH

<b>
1
signers
signer
</b>
</p>
</div>
Copy link

Choose a reason for hiding this comment

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

Ensure that the change from "signers" to "signer" does not impact any logic that depends on the plurality of signers. Verify surrounding code for logic that might assume a list or count of signers, and adjust business logic if applicable.

Expand Down
Loading