feat(Input): new promo component and tokens update #1441
Conversation
WalkthroughThis pull request introduces significant updates to the React component library, particularly focusing on the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (11)
packages/react-components/src/components/Input/types.ts (3)
3-6: Add JSDoc comments for the IInputIcon interface and its properties.Improve documentation by adding descriptions for the interface and its properties.
+/** + * Interface for configuring an icon within an input component + */ export interface IInputIcon { + /** + * React element to be rendered as the icon + */ source: React.ReactElement; + /** + * Position of the icon within the input + */ place: 'left' | 'right'; }
28-33: Document the dimensions for each input size.The size options would be clearer with specific dimensions or use cases for each variant.
/** - * Specify the input size + * Specify the input size + * - xsmall: [specify dimensions/use case] + * - compact: [specify dimensions/use case] + * - medium: [specify dimensions/use case] + * - large: [specify dimensions/use case] */
37-46: Consider alternative design approaches.Two potential issues:
- Required
mainClassNamemight be too restrictive. Consider making it optional and providing a default.isPromoboolean overlaps with the separateIInputPromoPropsinterface, which could lead to confusion.Consider using a discriminated union instead:
export type InputProps = | (IInputGlobalProps & { variant: 'default'; size?: InputSize }) | (IInputPromoProps & { variant: 'promo' });packages/react-components/src/components/Input/Input.module.scss (1)
35-43: Consider improvements to the promo variantTwo suggestions for the promo variant:
- Consider making the height configurable through a CSS variable for better maintainability
- The promo variant should have its own focus state styles to maintain consistency with the hover state customization
&--promo { border: 2px solid var(--input-promo-border-default); padding: 0 var(--spacing-4); - height: 52px; + height: var(--input-promo-height, 52px); &:hover { border-color: var(--input-promo-border-hover); } + + &:focus { + border-color: var(--input-promo-border-focus); + } }packages/react-components/src/components/Input/Input.spec.tsx (1)
Line range hint
15-106: Add test coverage for new promo variantThe test suite is comprehensive but missing coverage for the new promotional input variant mentioned in the PR objectives.
Please add test cases for:
- Promo variant rendering
- Promo-specific styling
- Promo-specific behavior
Would you like me to help draft the test cases for the promo variant?
packages/react-components/src/components/Input/Input.stories.tsx (2)
126-128: Consider adding args table configuration.The basic InputPromo story could benefit from the same argTypes configuration as the Default story to improve documentation.
+InputPromo.args = { + inputSize: 'medium', + placeholder: 'Placeholder text', +};
126-200: LGTM! Comprehensive story coverage.The new stories provide excellent coverage of the InputPromo component variants, following the same consistent pattern as the regular Input stories.
Consider adding JSDoc comments to describe promo-specific features and use cases.
packages/react-components/src/themes/light.scss (1)
215-215: Consider using the SCSS variable for consistency.The hardcoded value
#43434bshould reference$decor-gray600to maintain consistency with other color definitions.- --action-high-contrast-hover: #43434b; + --action-high-contrast-hover: #{$decor-gray600};packages/react-components/src/themes/dark.scss (1)
398-399: Use design tokens for color valuesConsider using the existing design tokens instead of hardcoded values:
- --input-promo-border-default: #62626d; - --input-promo-border-hover: #8d8d95; + --input-promo-border-default: var(--decor-gray500); + --input-promo-border-hover: var(--decor-gray300);Also, consider adding
--input-promo-border-disabledfor consistency with other input variants.packages/react-components/src/foundations/design-token.ts (1)
386-387: Add JSDoc comments to document the new tokensThe new promo input tokens follow the naming conventions, but would benefit from documentation.
+ /** Border color for promo input in default state */ InputPromoBorderDefault: '--input-promo-border-default', + /** Border color for promo input in hover state */ InputPromoBorderHover: '--input-promo-border-hover',packages/react-components/src/stories/foundations/Color/data.ts (1)
1479-1488: Add descriptions for the new color tokens.The implementation looks good, but please add meaningful descriptions to maintain consistency with other tokens in the
ColorsDataobject.Apply this diff:
InputPromoBorderDefault: { group: ColorGroup.BorderBasic, - desc: '', + desc: 'Default border color for promotional input component', deprecated: false, }, InputPromoBorderHover: { group: ColorGroup.BorderBasic, - desc: '', + desc: 'Hover state border color for promotional input component', deprecated: false, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
packages/react-components/src/components/AutoComplete/types.ts(2 hunks)packages/react-components/src/components/FormField/FormField.stories.tsx(2 hunks)packages/react-components/src/components/Input/Input.module.scss(2 hunks)packages/react-components/src/components/Input/Input.spec.tsx(1 hunks)packages/react-components/src/components/Input/Input.stories.tsx(3 hunks)packages/react-components/src/components/Input/Input.tsx(5 hunks)packages/react-components/src/components/Input/types.ts(1 hunks)packages/react-components/src/foundations/color-scheme.css(0 hunks)packages/react-components/src/foundations/design-token.ts(1 hunks)packages/react-components/src/foundations/shadow.css(2 hunks)packages/react-components/src/stories/foundations/Color/data.ts(1 hunks)packages/react-components/src/themes/dark.scss(1 hunks)packages/react-components/src/themes/light.scss(2 hunks)
💤 Files with no reviewable changes (1)
- packages/react-components/src/foundations/color-scheme.css
🔇 Additional comments (20)
packages/react-components/src/components/Input/types.ts (1)
8-26: LGTM! Well-structured base interface with clear documentation.
The interface provides a solid foundation for input components with essential props and proper JSDoc comments.
packages/react-components/src/components/AutoComplete/types.ts (2)
5-5: LGTM! Interface naming follows TypeScript conventions
The change to IInputProps aligns with TypeScript interface naming conventions and provides better module organization by importing directly from the types file.
16-16: Verify the interface compatibility across components
The change from InputProps to IInputProps looks good, but let's ensure all consuming components are updated.
✅ Verification successful
All consuming components are correctly using IInputProps
The codebase shows consistent usage of IInputProps. The AutoCompleteProps interface correctly extends from IInputProps, and all other components that need this interface are importing and using it properly. No instances of an old InputProps interface were found that would conflict with this change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining usages of old InputProps interface
# and verify IInputProps usage consistency
# Search for old InputProps usage
echo "Checking for old InputProps usage..."
rg "InputProps" --type ts
# Search for new IInputProps usage to verify consistency
echo "Verifying IInputProps usage..."
rg "IInputProps" --type ts
Length of output: 6676
packages/react-components/src/components/Input/Input.module.scss (1)
1-3: LGTM! Good use of SCSS variable for maintainability
The introduction of $base-class variable makes the component's base class name configurable from a single location.
packages/react-components/src/components/FormField/FormField.stories.tsx (2)
5-6: LGTM: Clean import restructuring
The separation of types into a dedicated types file follows best practices for better code organization.
30-32: LGTM: Type definition update aligns with new input system
The switch to IInputProps maintains consistency with the new promo component changes.
✅ Verification successful
Type definition is correctly used and consistent
The usage of IInputProps in FormField.stories is properly imported from Input/types and aligns with the component's type system. The interface extends IInputGlobalProps with an additional inputSize prop, making it the correct choice for the Input component usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent usage of IInputProps across the codebase
ast-grep --pattern 'IInputProps'
Length of output: 550
Script:
#!/bin/bash
# Check the content of Input/types.ts to verify IInputProps definition
cat packages/react-components/src/components/Input/types.ts
# Check if there are any other input-related interfaces that might conflict
rg "interface.*Input" --type ts
Length of output: 2058
packages/react-components/src/foundations/shadow.css (3)
58-58: Good contrast adjustment for dark theme
The increased opacity (25%) for active field state in dark theme provides better visibility while maintaining the design aesthetic.
Line range hint 10-10: Verify impact of focus shadow color change
The focus shadow color change (#4284f5 → #0059e1) appears to be more accessible, but let's verify it doesn't break existing components.
✅ Verification successful
Focus shadow color change is safe and consistent
The new color #0059e1 is already being used as the focus shadow color in the light theme, and multiple components are correctly using the --shadow-focus CSS variable. The change maintains design system consistency.
- Components using focus shadow:
- Switch
- OnboardingChecklist
- Checkbox
- Button
- ActionCard
- Accordion
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any components that might be affected by the focus shadow change
rg --type css "4284f5"
Length of output: 22
Script:
#!/bin/bash
# Search for any components that might be affected by the focus shadow color
rg --type css "0059e1|#0059e1"
# Also search for any shadow-related CSS variables that might reference focus shadows
rg --type css "shadow.*focus|focus.*shadow"
Length of output: 2399
27-27: Verify contrast ratio for active field state
The 15% opacity for active field state in light theme might be too subtle for some users.
✅ Verification successful
Active field state opacity values are appropriately set for both themes
The opacity values are well-considered:
- Light theme: 15% opacity provides subtle but visible feedback
- Dark theme: 25% opacity ensures better visibility in dark mode
- Consistent with existing shadow patterns in the codebase
- Follows accessibility guidelines found in documentation regarding contrast and keyboard interactions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any accessibility guidelines or contrast ratio requirements in the codebase
rg -i "contrast|a11y|accessibility" "*.md"
Length of output: 89
Script:
#!/bin/bash
# Search for accessibility-related content in all markdown files recursively
fd -e md -x rg -i "contrast|a11y|accessibility" {}
# Check for any design tokens or guidelines documentation
fd -e md -g "*design*" -g "*token*" -g "*style*" -g "*theme*" -x rg -i "contrast|color|opacity" {}
# Look for any test files related to these design tokens
fd -e test.js -e test.ts -e spec.js -e spec.ts -x rg -i "shadow|active.*field|focus" {}
# Check if there are similar opacity values used elsewhere in CSS
fd -e css -x rg -i "opacity|rgb.*\/\s*[0-9]+%" {}
Length of output: 2904
Script:
#!/bin/bash
# Check for opacity values in CSS files with fixed regex
fd -e css -x rg -l "opacity:|rgb.*\([^)]*\/\s*\d+%\)" {}
# Look for similar active states in CSS
fd -e css -x rg "active.*field|--state-active" {}
# Check the shadow.css file content
cat packages/react-components/src/foundations/shadow.css
Length of output: 5048
packages/react-components/src/components/Input/Input.spec.tsx (2)
9-10: LGTM! Clean type import update
12-13: Verify type consistency across test helpers
The type update looks good, but let's ensure all test utilities are updated consistently.
✅ Verification successful
Type consistency verified across input components
Each input component follows its own consistent type naming convention:
- Input:
IInputProps - SearchInput:
ISearchInputProps - NumericInput:
NumericInputProps - TagInput:
TagInputProps<T>
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining uses of InputProps in test files
rg "InputProps" --type ts --type tsx | grep -i "spec\|test"
Length of output: 87
Script:
#!/bin/bash
# Search for test files first
fd ".*\.(spec|test)\.(ts|tsx)$"
# Then search for InputProps pattern in those files
fd ".*\.(spec|test)\.(ts|tsx)$" --exec rg -l "InputProps" {}
Length of output: 5685
Script:
#!/bin/bash
# Check the actual usage context of InputProps in the identified files
rg -B2 -A2 "InputProps" ./packages/react-components/src/components/Input/Input.spec.tsx ./packages/react-components/src/components/TagInput/TagInput.spec.tsx ./packages/react-components/src/components/SearchInput/SearchInput.spec.tsx ./packages/react-components/src/components/NumericInput/NumericInput.spec.tsx
Length of output: 4613
packages/react-components/src/components/Input/Input.stories.tsx (2)
9-10: LGTM! Clean import structure and consistent type naming.
The aliasing of InputPromo to InputPromoComponent improves clarity, and the interface naming follows the 'I' prefix convention.
31-31: LGTM! Consistent type usage.
Type update aligns with the new interface naming convention.
packages/react-components/src/themes/light.scss (1)
400-401: LGTM! New promo input border properties.
The values align well with the existing color system and support the new promo input component requirements.
packages/react-components/src/components/Input/Input.tsx (6)
13-18: Imports restructured for better type management.
Importing all the necessary types from './types' improves code organization and maintainability.
Line range hint 24-30: renderIcon function correctly utilizes IInputIcon interface.
The renderIcon function properly clones the icon element with appropriate class names based on its placement and disability state.
Line range hint 36-85: Enhanced InputComponent with new props for flexibility.
Adding mainClassName and isPromo props to InputComponent increases its versatility, allowing for customized styling and promotional variants.
85-85: Conditional text size aligns with promo requirements.
Setting the Text component's size based on isPromo ensures that promotional inputs have the intended larger text size.
122-133: Input component simplifies size-based styling.
The Input component now wraps InputComponent, applying mainClassName based on inputSize, which streamlines the usage of input sizes throughout the codebase.
137-150: Introduced InputPromo for promotional input variants.
The new InputPromo component effectively applies promotional styles and sets isPromo, fulfilling the PR's objective to create a promo variant.
packages/react-components/src/components/Input/Input.module.scss
Outdated
Show resolved
Hide resolved
|
Need to add shadow when filed is active (typing mode or focussed) shadow |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/react-components/src/components/Input/Input.tsx (2)
Line range hint
24-34: Add return type annotation for better type safetyThe renderIcon function should explicitly declare its return type for better maintainability.
-const renderIcon = (icon: IInputIcon, disabled?: boolean) => +const renderIcon = (icon: IInputIcon, disabled?: boolean): JSX.Element =>
Line range hint
85-89: Fix tabIndex attribute nameThe HTML attribute should be
tabIndex(camelCase) instead oftab-index(kebab-case) in React.- tab-index="0" + tabIndex={0}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/react-components/src/components/Input/Input.tsx(5 hunks)
🔇 Additional comments (3)
packages/react-components/src/components/Input/Input.tsx (3)
13-18: LGTM! Clean type organization
Good separation of concerns by moving interfaces to a separate types file.
122-133: LGTM! Clean wrapper implementation
Good use of default props and proper component composition.
135-150: Verify shadow effect implementation
Per vladko-uxds's feedback, ensure the promo variant includes the requested shadow effect on focus.
✅ Verification successful
Shadow effect is properly implemented
The promo variant includes the requested shadow effect on focus:
&--promo {
&.#{$base-class}--focused,
&.#{$base-class}--focused:hover {
box-shadow: var(--state-active-field);
}
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for shadow-related CSS properties in the styles
rg -g '*.scss' -g '*.css' 'box-shadow.*promo.*focused'
Length of output: 56
Script:
#!/bin/bash
# Let's try a broader search for shadow styles in promo variant
rg -g '*.scss' -g '*.css' '\.promo.*box-shadow'
# Also search for the specific class name pattern
rg -g '*.scss' -g '*.css' "${baseClass}--promo.*box-shadow"
# Search for any shadow properties in the component's style file
fd -e scss -e css "Input" --exec rg "box-shadow" {}
Length of output: 640
Script:
#!/bin/bash
# Let's find and check the Input component's style file content
fd -e scss -e css "Input" --exec cat {}
# Also search for any focus-related styles
fd -e scss -e css "Input" --exec rg "focus|focused" {}
Length of output: 9769
Resolves: #1345, #1420
Description
New promo variant as a separate component.
Tokens update.
Storybook
https://feature-1345--613a8e945a5665003a05113b.chromatic.com
Checklist
Obligatory:
Summary by CodeRabbit
Release Notes
New Features
InputPromocomponent with promotional styling options.InputPromoshowcasing various states and types.Enhancements
Bug Fixes
Chores