-
Notifications
You must be signed in to change notification settings - Fork 1
adding more tests #45
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.
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
Changes from 5 commits
18a6faf
ecb5a9a
18d4b4b
2699b8f
16c2a2c
701ebcf
c77fded
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,6 +43,16 @@ export default defineConfig({ | |
| testMatch: '**/mask.spec.ts', | ||
| dependencies: ['base tests'], | ||
| }, | ||
| { | ||
| name: 'controlled-value tests', | ||
| testMatch: '**/controlled-value.spec.ts', | ||
| dependencies: ['base tests'], | ||
| }, | ||
| { | ||
| name: 'input-type tests', | ||
| testMatch: '**/input-type.spec.ts', | ||
| dependencies: ['base tests'], | ||
| }, | ||
|
Comment on lines
+46
to
+55
|
||
| { | ||
| name: 'chromium', | ||
| use: { ...devices['Desktop Chrome'] }, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -36,6 +36,7 @@ type CurrencyInputState = { | |||||||||||||||
| disableSelectionHandling: boolean, | ||||||||||||||||
| maskedValue: string, | ||||||||||||||||
| value: number | string, // TODO: should be string? should also have a separate float field for 'pennies' | ||||||||||||||||
| previousProps?: Readonly<CurrencyInputProps>, // Track previous props to detect changes | ||||||||||||||||
| }; | ||||||||||||||||
|
|
||||||||||||||||
| type SelectionSnapshot = { | ||||||||||||||||
|
|
@@ -124,22 +125,23 @@ class CurrencyInput extends React.Component<CurrencyInputProps, CurrencyInputSta | |||||||||||||||
| * General function used to cleanup and define the final props used for rendering | ||||||||||||||||
| * @returns CurrencyInputState | ||||||||||||||||
| */ | ||||||||||||||||
| static prepareProps({ | ||||||||||||||||
| onChangeEvent, | ||||||||||||||||
| value: propValue, | ||||||||||||||||
| decimalSeparator, | ||||||||||||||||
| thousandSeparator, | ||||||||||||||||
| precision, | ||||||||||||||||
| inputType, | ||||||||||||||||
| allowNegative, | ||||||||||||||||
| allowEmpty, | ||||||||||||||||
| prefix, | ||||||||||||||||
| suffix, | ||||||||||||||||
| selectAllOnFocus, | ||||||||||||||||
| autoFocus, | ||||||||||||||||
| disableSelectionHandling: propDisableSelectionHandling, | ||||||||||||||||
| ...customProps | ||||||||||||||||
| }: Readonly<CurrencyInputProps>): CurrencyInputState { | ||||||||||||||||
| static prepareProps(props: Readonly<CurrencyInputProps>): CurrencyInputState { | ||||||||||||||||
| const { | ||||||||||||||||
| onChangeEvent, | ||||||||||||||||
| value: propValue, | ||||||||||||||||
| decimalSeparator, | ||||||||||||||||
| thousandSeparator, | ||||||||||||||||
| precision, | ||||||||||||||||
| inputType, | ||||||||||||||||
| allowNegative, | ||||||||||||||||
| allowEmpty, | ||||||||||||||||
| prefix, | ||||||||||||||||
| suffix, | ||||||||||||||||
| selectAllOnFocus, | ||||||||||||||||
| autoFocus, | ||||||||||||||||
| disableSelectionHandling: propDisableSelectionHandling, | ||||||||||||||||
| ...customProps | ||||||||||||||||
| } = props; | ||||||||||||||||
| let initialValue = propValue; | ||||||||||||||||
| if (initialValue === null) { | ||||||||||||||||
| initialValue = allowEmpty ? null : ''; | ||||||||||||||||
|
|
@@ -166,7 +168,7 @@ class CurrencyInput extends React.Component<CurrencyInputProps, CurrencyInputSta | |||||||||||||||
| ); | ||||||||||||||||
|
|
||||||||||||||||
| const disableSelectionHandling = propDisableSelectionHandling || inputType === 'number'; | ||||||||||||||||
| return { maskedValue, value, customProps, disableSelectionHandling }; | ||||||||||||||||
| return { maskedValue, value, customProps, disableSelectionHandling, previousProps: props }; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| /** | ||||||||||||||||
|
|
@@ -182,11 +184,58 @@ class CurrencyInput extends React.Component<CurrencyInputProps, CurrencyInputSta | |||||||||||||||
| * @see https://reactjs.org/docs/react-component.html#static-getderivedstatefromprops | ||||||||||||||||
| */ | ||||||||||||||||
| static getDerivedStateFromProps(nextProps: Readonly<CurrencyInputProps>, prevState: Readonly<CurrencyInputState>) { | ||||||||||||||||
| const props = { ...nextProps }; | ||||||||||||||||
| if (nextProps.value !== prevState.value) { | ||||||||||||||||
| props.value = prevState.value; | ||||||||||||||||
| const previousProps = prevState.previousProps || nextProps; // First call uses the initial props snapshot | ||||||||||||||||
|
|
||||||||||||||||
| // Check if the VALUE prop itself changed (parent is controlling the input) | ||||||||||||||||
| const valueChanged = nextProps.value !== previousProps.value; | ||||||||||||||||
|
|
||||||||||||||||
| // Check if separators or display formatting changed (these require reformatting the current value) | ||||||||||||||||
| const formatChanged = | ||||||||||||||||
| nextProps.decimalSeparator !== previousProps.decimalSeparator || | ||||||||||||||||
| nextProps.thousandSeparator !== previousProps.thousandSeparator || | ||||||||||||||||
| nextProps.precision !== previousProps.precision || | ||||||||||||||||
| nextProps.prefix !== previousProps.prefix || | ||||||||||||||||
| nextProps.suffix !== previousProps.suffix; | ||||||||||||||||
|
||||||||||||||||
| nextProps.suffix !== previousProps.suffix; | |
| nextProps.suffix !== previousProps.suffix || | |
| nextProps.inputType !== previousProps.inputType; |
Copilot
AI
Dec 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sequential if-statements mean that if multiple prop changes occur simultaneously, only the first matching condition is handled. For example, if both format props (like prefix) and allowNegative change at the same time, the formatChanged block executes and returns early, bypassing the special allowNegativeChanged logic that converts negative values to positive. Consider checking allowNegativeChanged before formatChanged, or include allowNegative in the formatChanged condition so that the special negative-to-positive conversion logic is always applied when allowNegative becomes false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The allowNegativeChanged check only handles the case when allowNegative is disabled. If allowNegative is enabled (changed from false to true), the function doesn't update previousProps and falls through to line 229 which preserves the state. However, when other props like inputType change together with allowNegative being enabled, this could lead to stale previousProps. Consider adding an else clause or ensuring previousProps is always updated when allowNegativeChanged is true.
| } | |
| } | |
| // Even if value is not negative, still update previousProps to avoid staleness | |
| return { ...prevState, previousProps: nextProps }; | |
| } else if (allowNegativeChanged) { | |
| // allowNegative was enabled; update previousProps to avoid staleness | |
| return { ...prevState, previousProps: nextProps }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new 'controlled-value tests' project doesn't specify any dependencies, unlike other test projects like 'mask tests' which depends on 'base tests'. If there are any setup requirements or if these tests should run after base tests complete, consider adding a dependencies array to ensure proper test execution order.