Skip to content

Conversation

GrabowskiM
Copy link
Contributor

🎫 Issue IBX-10307

Description:

For QA:

Documentation:

@GrabowskiM GrabowskiM requested review from a team and Copilot August 19, 2025 14:39
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a Form Control wrapper for Input Text components, implementing a reusable base form control structure with label and helper text functionality.

Key changes:

  • New BaseFormControl component providing consistent form field layout with label and helper text support
  • FormControlInputText component that wraps basic InputText with validation and form control features
  • Enhanced validation system with automatic required field validation

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
packages/components/src/internal/partials/BaseFormControl/* New base form control component providing consistent layout structure
packages/components/src/internal/hooks/useValidatorManager.ts Added default generic type parameter for better type safety
packages/components/src/formControls/InputText/* New form control wrapper for InputText with validation and structured layout
packages/components/src/inputs/InputText/InputText.test.stories.tsx Added value assertion to existing test
packages/assets/src/scss/* Added form control styles and reorganized imports

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

onValidate = () => undefined,
value = '',
}: FormControlInputTextProps) => {
const validatorManager = useValidatorManager<string | number>();
Copy link
Preview

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

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

The validatorManager is created on every render. Consider memoizing it with useMemo to avoid unnecessary recreations when props change.

Suggested change
const validatorManager = useValidatorManager<string | number>();
const validatorManager = useMemo(() => useValidatorManager<string | number>(), []);

Copilot uses AI. Check for mistakes.

validatorManager.removeValidator(isEmptyValidator);
};
}
}, [input.required]);
Copy link
Preview

Copilot AI Aug 19, 2025

Choose a reason for hiding this comment

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

The dependency array should include validatorManager to ensure the effect has access to the current instance. Missing this dependency could lead to stale closures.

Suggested change
}, [input.required]);
}, [input.required, validatorManager]);

Copilot uses AI. Check for mistakes.

@@ -5,7 +5,7 @@ import { TranslatorContext } from '@ids-context/Translator';
import BaseValidator from '../../validators/BaseValidator';
import ValidatorManager from '../../validators/ValidatorManager';

export default <T>(validators: BaseValidator<T>[] = []) => {
export default <T = never>(validators: BaseValidator<T>[] = []) => {
const translator = useContext(TranslatorContext);
const validatorManagerInstance = useRef(new ValidatorManager<T>(validators, { translator }));

Choose a reason for hiding this comment

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

Why not? It ensures changes when validators or translators change.

Suggested change
const validatorManagerInstance = useRef(new ValidatorManager<T>(validators, { translator }));
const validatorManagerInstance = useMemo(() => new ValidatorManager<T>(validators, { translator }), [validators, translator]);

Base automatically changed from IBX-10471 to main August 25, 2025 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants