fix(form): modify mobile form problem #2643
Conversation
|
[e2e-test-warn] The title of the Pull request should look like "fix(vue-renderless): [action-menu, alert] fix xxx bug". Please make sure you've read our contributing guide |
WalkthroughThe changes in this pull request focus on enhancing the handling of the form state and rendering logic within mobile components. Key modifications include the transition from Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 3
🧹 Outside diff range and nitpick comments (9)
packages/mobile/components/input/src/input.ts (1)
128-131: Add unit tests for the newtitlepropertyThe new
titleproperty adds functionality that should be tested to ensure it behaves as expected.Would you like assistance in creating unit tests for the
titleproperty?packages/mobile/components/form-item/src/renderless/index.ts (4)
124-137: Consider adding type guards for better type safety.While the refactoring to
formInstanceis correct, consider adding type guards to handle potential undefined values more explicitly.- if (state.formInstance.labelPosition === POSITION.Top || state.formInstance.inline) { + if (!state.formInstance?.labelPosition) return result + if (state.formInstance.labelPosition === POSITION.Top || state.formInstance.inline) {
Line range hint
214-320: Add explicit error handling for invalid model paths.While the path handling logic is correct, consider adding explicit error handling for cases where the model path is invalid.
const model = state.formInstance?.model if (!model || !props.prop) { + console.warn('[Form-Item] Invalid model or prop') return }
Line range hint
489-506: Add JSDoc and type safety improvements for tooltip handling.While the tooltip functionality is correct, consider adding proper JSDoc documentation and type safety improvements.
+/** + * Handles mouse enter events for form items + * @param {MouseEvent} e - The mouse event + */ export const handleMouseenter = ({ state }: Pick<IFormItemRenderlessParams, 'state'>) => (e): void => { - if (!state.isDisplayOnly || !state.typeName || !state.formInstance) return + if (!state.isDisplayOnly || !state.typeName || !state.formInstance?.showTooltip) return🧰 Tools
🪛 Biome (1.9.4)
[error] 487-488: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
513-521: LGTM! Consider strengthening null checks.The tooltip functionality for labels is properly implemented. Consider adding stronger null checks for better reliability.
- if (!state.formInstance?.overflowTitle || !state.formInstance || slots.label) return + if (!state.formInstance?.overflowTitle || !state.formInstance?.showTooltip || slots.label) returnpackages/theme-mobile/src/form-item/vars.less (1)
2-3: Consider adding fallback values for CSS variablesWhile the new mobile-specific color variables improve separation of concerns, consider adding fallback values to ensure graceful degradation:
- --ti-mobile-form-item-label-color: #333; - --ti-mobile-form-item-error-color: #f5222d; + --ti-mobile-form-item-label-color: var(--ti-common-color-text-primary, #333); + --ti-mobile-form-item-error-color: var(--ti-common-color-error, #f5222d);packages/theme-mobile/src/form/index.less (1)
55-55: Consider responsive margins for different screen sizesThe fixed margin-right value of 10px might not scale well across different screen sizes. Consider using relative units or media queries for better responsiveness.
- margin-right: 10px; + margin-right: clamp(8px, 2vw, 16px);packages/theme-mobile/src/form-item/index.less (2)
41-41: Consider using relative units for line heightsFixed pixel values for line heights might not scale well with different font sizes or screen densities. Consider using relative units for better accessibility and responsiveness.
- line-height: 36px; + line-height: 2.25; /* 36px ÷ 16px */ - line-height: 28px; + line-height: 1.75; /* 28px ÷ 16px */ - line-height: 24px; + line-height: 1.5; /* 24px ÷ 16px */Also applies to: 46-46, 55-55
93-93: Consider using relative font sizeFixed pixel values for font sizes might affect readability across different devices and user preferences.
- font-size: 14px; + font-size: 0.875rem; /* 14px ÷ 16px */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
packages/mobile/components/form-item/src/mobile.vue(7 hunks)packages/mobile/components/form-item/src/renderless/index.ts(8 hunks)packages/mobile/components/form-item/src/renderless/vue.ts(2 hunks)packages/mobile/components/form/src/renderless/vue.ts(1 hunks)packages/mobile/components/input/src/input.ts(2 hunks)packages/theme-mobile/src/form-item/index.less(8 hunks)packages/theme-mobile/src/form-item/vars.less(1 hunks)packages/theme-mobile/src/form/index.less(1 hunks)
🔇 Additional comments (10)
packages/mobile/components/form-item/src/mobile.vue (3)
13-13: Approved change to import statement
The import statement correctly includes the necessary modules.
53-53: Confirm hardcoding isMobile to true
Hardcoding isMobile to true may prevent this component from adapting to non-mobile contexts. Ensure this change is intentional and won't affect any conditional logic based on isMobile.
167-167: Ensure label suffix handles undefined formInstance
When appending state.formInstance?.labelSuffix, confirm that labelSuffix is correctly defined or provide a default value to prevent potential issues.
packages/mobile/components/form/src/renderless/vue.ts (1)
111-111: Verify impact of changing provide('form', vm)
Changing the provided value from parent to vm may affect components that inject form. Ensure all dependent components expect vm as the form instance.
packages/mobile/components/input/src/input.ts (1)
16-30: Approved re-import of utility functions
Re-importing utility functions ensures all necessary methods are available for the input component.
packages/mobile/components/form-item/src/renderless/vue.ts (1)
111-111: Approved use of optional chaining for safer property access
Using optional chaining with state.formInstance enhances robustness by preventing runtime errors when formInstance may be undefined.
Also applies to: 121-121
packages/mobile/components/form-item/src/renderless/index.ts (3)
100-105: LGTM! Width calculation logic is preserved.
The transition to formInstance maintains the same width calculation logic while improving consistency.
354-354: LGTM! Rules access properly updated.
The transition to formInstance for rules access is consistent with the overall refactoring pattern.
82-86: LGTM! Verify formInstance references.
The refactoring from form to formInstance is consistent. The logic remains unchanged and proper optional chaining is maintained.
✅ Verification successful
Consistent usage of formInstance property confirmed
The codebase scan shows that formInstance is consistently used across the form-item component files, replacing the previous form references. The changes in the reviewed code segment align with this pattern, showing proper optional chaining and maintaining the same logical structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent usage of formInstance across the codebase
rg -g '*.{vue,ts,js}' "formInstance\." packages/mobile/components/form-item/
Length of output: 3412
packages/theme-mobile/src/form-item/index.less (1)
76-76: Verify color contrast ratios for accessibility
The new color variables are being used consistently across different states. However, ensure that the chosen colors meet WCAG contrast requirements for accessibility.
Also applies to: 121-121, 142-142, 152-152, 163-163, 169-169
| @@ -294,9 +208,9 @@ export default defineComponent({ | |||
| style: !isMobile && state.contentStyle | |||
| }, | |||
| [ | |||
| isMobile ? LabelContent : null, | |||
| LabelContent, | |||
| FormItemContnet, | |||
There was a problem hiding this comment.
Fix typo in FormItemContnet variable name
The variable FormItemContnet appears to be misspelled. It should likely be FormItemContent.
- FormItemContnet,
+ FormItemContent,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| FormItemContnet, | |
| FormItemContent, |
| ) | ||
| ] | ||
| ) | ||
| const validatePosition = this.validatePosition || state.formInstance.validatePosition || 'right' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add optional chaining to formInstance.validatePosition
To avoid runtime errors if formInstance is undefined, use optional chaining when accessing validatePosition.
- const validatePosition = this.validatePosition || state.formInstance.validatePosition || 'right'
+ const validatePosition = this.validatePosition || state.formInstance?.validatePosition || 'right'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const validatePosition = this.validatePosition || state.formInstance.validatePosition || 'right' | |
| const validatePosition = this.validatePosition || state.formInstance?.validatePosition || 'right' |
| const classPrefix = isMobile ? 'tiny-mobile-' : 'tiny-' | ||
| const labelSlot = slots.label ? slots.label() : null | ||
| const defaultSlots = slots.default ? slots.default() : null | ||
| const errorSlot = scopedSlots.error && scopedSlots.error(state.validateMessage) | ||
| const formItemClass = `${classPrefix}form-item--${state.sizeClass ? state.sizeClass : 'default'}` | ||
| const isShowError = state.validateState === 'error' && showMessage && state.form.showMessage | ||
| const isShowError = state.validateState === 'error' && showMessage && formInstance.showMessage |
There was a problem hiding this comment.
Use optional chaining with formInstance.showMessage
Accessing formInstance.showMessage without checking if formInstance is defined may lead to runtime errors. Consider using optional chaining to prevent potential issues.
- const isShowError = state.validateState === 'error' && showMessage && formInstance.showMessage
+ const isShowError = state.validateState === 'error' && showMessage && formInstance?.showMessage📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isShowError = state.validateState === 'error' && showMessage && formInstance.showMessage | |
| const isShowError = state.validateState === 'error' && showMessage && formInstance?.showMessage |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (7)
packages/theme-mobile/src/form/index.less (1)
55-55: Consider maintaining customization capabilityThe change from
var(--ti-form-item-margin-right, 10px)to a fixed10pxaligns with other CSS variable removals, but:
- Reduces customization flexibility
- May impact responsive designs where different spacings are needed
Consider keeping the variable with a default value:
- margin-right: 10px; + margin-right: var(--ti-mobile-form-item-margin-right, 10px);packages/mobile/components/form/src/renderless/vue.ts (1)
Line range hint
1-111: Consider documenting the mobile form architecture changesThe changes across these files show a coordinated effort to:
- Simplify mobile form styling by removing CSS variables
- Change form context management
- Fix mobile-specific form issues
However, there are some architectural considerations:
- Document the rationale behind removing CSS variables and impact on customization
- Add tests for the new form context propagation
- Update component documentation to reflect these changes
Consider creating an architecture decision record (ADR) to document:
- The motivation behind these changes
- The trade-offs considered
- The impact on customization and maintainability
packages/theme-mobile/src/form-item/index.less (2)
41-41: Consider using relative units for line heightsWhile the fixed pixel values (36px, 28px, 24px) work for most cases, using relative units (like
remorem) would better support accessibility and different screen densities.- line-height: 36px; + line-height: 2.25rem; /* 36px at 16px base */ - line-height: 28px; + line-height: 1.75rem; /* 28px at 16px base */ - line-height: 24px; + line-height: 1.5rem; /* 24px at 16px base */Also applies to: 46-46, 55-55
93-93: Consider using a CSS variable for font sizeHardcoding the font size to 14px reduces flexibility. Consider using a CSS variable to maintain consistency and support theming.
- font-size: 14px; + font-size: var(--ti-mobile-form-item-content-font-size, 14px);packages/mobile/components/form-item/src/mobile.vue (1)
167-167: Handle undefined labelSuffix gracefullyThe optional chaining is good, but consider providing a default value for undefined cases.
- labelSlot || label + (state.formInstance?.labelSuffix || '') + labelSlot || label + (state.formInstance?.labelSuffix ?? '')packages/mobile/components/form-item/src/renderless/index.ts (2)
Line range hint
489-521: Add consistent optional chaining across tooltip-related methods.Several formInstance accesses lack optional chaining, which could cause runtime errors. Also, the null checks could be simplified.
Apply these improvements:
-if (!state.isDisplayOnly || !state.typeName || !state.formInstance) return +if (!state.isDisplayOnly || !state.typeName || !state.formInstance?.showTooltip) return -state.formInstance.showTooltip(dom, state.displayedValue) +state.formInstance?.showTooltip(dom, state.displayedValue) -if (!state.formInstance?.overflowTitle || !state.formInstance || slots.label) return +if (!state.formInstance?.overflowTitle || slots.label) return -state.formInstance.showTooltip(label, props.label + state.formInstance.labelSuffix) +state.formInstance?.showTooltip(label, props.label + state.formInstance?.labelSuffix) -state.formInstance && state.formInstance.hideTooltip() +state.formInstance?.hideTooltip()
Line range hint
1-549: Add unit tests for the formInstance migration.The PR objectives indicate that tests haven't been added. Given the significant changes in form instance handling and the addition of null safety checks, it's important to add unit tests to verify the behavior, especially for edge cases where formInstance might be null.
Would you like me to help generate unit tests for these changes?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
packages/mobile/components/form-item/src/mobile.vue(7 hunks)packages/mobile/components/form-item/src/renderless/index.ts(8 hunks)packages/mobile/components/form-item/src/renderless/vue.ts(2 hunks)packages/mobile/components/form/src/renderless/vue.ts(1 hunks)packages/mobile/components/input/src/input.ts(2 hunks)packages/theme-mobile/src/form-item/index.less(8 hunks)packages/theme-mobile/src/form-item/vars.less(1 hunks)packages/theme-mobile/src/form/index.less(1 hunks)
🔇 Additional comments (11)
packages/mobile/components/input/src/input.ts (3)
16-29: LGTM! Well-organized type imports.
The consolidation of renderless function types into a single import block improves code organization and maintainability.
Line range hint 207-219: LGTM! Comprehensive type definitions.
The IInputApi interface properly maps all imported function types, ensuring type safety and maintainability.
128-131: Clarify the relationship between title-related props.
The component now has three title-related props:
title(new)textareaTitleshowTitle
This might cause confusion for developers. Consider:
- Adding JSDoc comments to clarify the purpose of each title prop
- Updating component documentation
- Verifying the template implementation
packages/theme-mobile/src/form-item/vars.less (1)
2-3: Document the migration of removed CSS variables
While the new mobile-specific color variables are good additions, the removal of several CSS variables (line-height and font-size) should be documented to help other developers understand:
- Where the fixed values are now defined
- Why the variables were removed
- Impact on customization capabilities
Let's check for any remaining references to the removed variables:
✅ Verification successful
No references found to removed CSS variables - safe to proceed
The verification shows that there are no remaining references to the removed CSS variables in either Less files or Vue components. This indicates that the removal of these variables is safe and won't cause any broken references in the codebase. While documentation would be helpful for future reference, it's not critical since the variables are completely unused.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to removed variables
rg --type=less --type=vue \
"ti-form-item-(small|medium|mini)-line-height|ti-form-item-label-(line-height|font-size)|ti-form-item-error-font-size"
Length of output: 174
Script:
#!/bin/bash
# Search for any remaining references to removed variables, fixing the file type issue
rg --type=less \
"ti-form-item-(small|medium|mini)-line-height|ti-form-item-label-(line-height|font-size)|ti-form-item-error-font-size"
# Also search in .vue files separately
find . -name "*.vue" -type f -exec grep -l \
"ti-form-item-\(small\|medium\|mini\)-line-height\|ti-form-item-label-\(line-height\|font-size\)\|ti-form-item-error-font-size" \
{} \;
Length of output: 317
packages/mobile/components/form/src/renderless/vue.ts (1)
111-111:
Verify form context propagation
The change from parent to vm as the provided form context is significant and could affect child components' behavior. This change:
- May fix the mobile form issue mentioned in the PR
- Could impact form validation and state management
Let's verify the form context usage in child components:
Please add tests to verify:
- Form validation still works correctly
- Child components can access form methods
- No regression in form functionality
Would you like me to help create test cases for these scenarios?
✅ Verification successful
Form context change appears safe and well-integrated
The codebase analysis shows that the form context change is consistent with the existing pattern across components:
- Form-items correctly inject and use the form context (
packages/mobile/components/form-item/src/renderless/vue.ts) - Multiple form-related components (input, checkbox, radio, etc.) follow a consistent pattern of injecting the form context with a null fallback
- The form context is used similarly in both mobile and desktop components
- Components handle form context appropriately for validation and state management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components that inject and use the form context
ast-grep --pattern 'inject($_, "form")'
# Search for potential form context usage
rg -A 3 'inject.*form'
Length of output: 11426
packages/theme-mobile/src/form-item/index.less (1)
76-76: LGTM! Good separation of mobile-specific styling
The consistent transition from ti-form-item to ti-mobile-form-item CSS variables improves the separation of concerns between mobile and desktop styles.
Also applies to: 121-121, 142-142, 152-152, 163-163, 169-169
packages/mobile/components/form-item/src/renderless/vue.ts (1)
111-111: LGTM! Improved null safety with optional chaining
The addition of optional chaining for accessing formInstance.state properties prevents potential runtime errors.
Also applies to: 121-121
packages/mobile/components/form-item/src/mobile.vue (1)
211-213: Consider adding error boundaries
The template renders multiple elements without error boundaries. Consider wrapping critical sections with error handling.
Also applies to: 222-222
packages/mobile/components/form-item/src/renderless/index.ts (3)
100-105: LGTM! Consistent property access pattern.
The changes maintain consistency with the formInstance migration pattern while adding null safety.
214-214: LGTM! Proper null safety added.
The optional chaining operator provides appropriate null safety for model access.
354-354: LGTM! Proper null safety added.
The optional chaining operator provides appropriate null safety for rules access.
| @@ -10,7 +10,7 @@ | |||
| * | |||
| --> | |||
| <script lang="tsx"> | |||
| import { $prefix, setup, parseVnode, h, defineComponent, isVue2, hooks } from '../../../vue-common' | |||
| import { $prefix, setup, parseVnode, h, defineComponent, isVue2 } from '../../../vue-common' | |||
There was a problem hiding this comment.
Tests needed for this bugfix
According to the PR objectives, this is a bugfix but no tests have been added. Please add tests to verify the form behavior on mobile devices, especially around:
- Validation message positioning
- Label suffix rendering
- Error state handling
Would you like me to help generate test cases for these scenarios?
| const validatePosition = this.validatePosition || state.formInstance.validatePosition || 'right' | ||
| validateMessage = state.validateMessage ? ( | ||
| validatePosition === 'right' ? ( | ||
| <div class="tiny-mobile-input-form__error align-right">{state.validateMessage}</div> | ||
| ) : ( | ||
| <div class="tiny-mobile-input-form__error align-left">{state.validateMessage}</div> | ||
| ) | ||
| ) : null |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for position value
The validation position logic should handle invalid position values gracefully.
- const validatePosition = this.validatePosition || state.formInstance.validatePosition || 'right'
+ const validPositions = ['right', 'left']
+ const requestedPosition = this.validatePosition || state.formInstance.validatePosition || 'right'
+ const validatePosition = validPositions.includes(requestedPosition) ? requestedPosition : 'right'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const validatePosition = this.validatePosition || state.formInstance.validatePosition || 'right' | |
| validateMessage = state.validateMessage ? ( | |
| validatePosition === 'right' ? ( | |
| <div class="tiny-mobile-input-form__error align-right">{state.validateMessage}</div> | |
| ) : ( | |
| <div class="tiny-mobile-input-form__error align-left">{state.validateMessage}</div> | |
| ) | |
| ) : null | |
| const validPositions = ['right', 'left'] | |
| const requestedPosition = this.validatePosition || state.formInstance.validatePosition || 'right' | |
| const validatePosition = validPositions.includes(requestedPosition) ? requestedPosition : 'right' | |
| validateMessage = state.validateMessage ? ( | |
| validatePosition === 'right' ? ( | |
| <div class="tiny-mobile-input-form__error align-right">{state.validateMessage}</div> | |
| ) : ( | |
| <div class="tiny-mobile-input-form__error align-left">{state.validateMessage}</div> | |
| ) | |
| ) : null |
| const { validateIcon, isErrorInline, isErrorBlock, tooltipType } = state | ||
| const isMobile = state.mode === 'mobile' | ||
| const { validateIcon, isErrorInline, isErrorBlock, formInstance } = state | ||
| const isMobile = true |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making mobile mode configurable
Hardcoding isMobile = true reduces component reusability. Consider making this configurable through props or environment settings.
- const isMobile = true
+ const isMobile = props.forceMobile ?? state.mode === 'mobile'Committable suggestion skipped: line range outside the PR's diff.
| @@ -338,7 +317,7 @@ export const resetField = | |||
| state.validateState = '' | |||
| state.validateMessage = '' | |||
|
|
|||
| let model = state.form.model || {} | |||
| let model = state.formInstance.model || {} | |||
There was a problem hiding this comment.
Add optional chaining to prevent potential runtime errors.
The formInstance access lacks optional chaining, which could cause runtime errors if formInstance is null.
Apply this diff to add null safety:
-let model = state.formInstance.model || {}
+let model = state.formInstance?.model || {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let model = state.formInstance.model || {} | |
| let model = state.formInstance?.model || {} |
| if (state.formInstance.labelPosition === POSITION.Top || state.formInstance.inline) { | ||
| return result | ||
| } | ||
|
|
||
| if (!label && !props.labelWidth && state.isNested) { | ||
| return result | ||
| } | ||
|
|
||
| const labelWidth = props.labelWidth || state.form.state.labelWidth | ||
| const labelWidth = props.labelWidth || state.formInstance.state.labelWidth | ||
|
|
||
| if (labelWidth === 'auto') { | ||
| if (props.labelWidth === 'auto') { | ||
| result.marginLeft = state.computedLabelWidth | ||
| } else if (state.form.state.labelWidth === 'auto') { | ||
| } else if (state.formInstance.state.labelWidth === 'auto') { |
There was a problem hiding this comment.
Add optional chaining to prevent potential runtime errors.
The formInstance access on line 124 lacks optional chaining, which could cause runtime errors if formInstance is null.
Apply this diff to add null safety:
-if (state.formInstance.labelPosition === POSITION.Top || state.formInstance.inline) {
+if (state.formInstance?.labelPosition === POSITION.Top || state.formInstance?.inline) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (state.formInstance.labelPosition === POSITION.Top || state.formInstance.inline) { | |
| return result | |
| } | |
| if (!label && !props.labelWidth && state.isNested) { | |
| return result | |
| } | |
| const labelWidth = props.labelWidth || state.form.state.labelWidth | |
| const labelWidth = props.labelWidth || state.formInstance.state.labelWidth | |
| if (labelWidth === 'auto') { | |
| if (props.labelWidth === 'auto') { | |
| result.marginLeft = state.computedLabelWidth | |
| } else if (state.form.state.labelWidth === 'auto') { | |
| } else if (state.formInstance.state.labelWidth === 'auto') { | |
| if (state.formInstance?.labelPosition === POSITION.Top || state.formInstance?.inline) { | |
| return result | |
| } | |
| if (!label && !props.labelWidth && state.isNested) { | |
| return result | |
| } | |
| const labelWidth = props.labelWidth || state.formInstance.state.labelWidth | |
| if (labelWidth === 'auto') { | |
| if (props.labelWidth === 'auto') { | |
| result.marginLeft = state.computedLabelWidth | |
| } else if (state.formInstance.state.labelWidth === 'auto') { |
| if (state.formInstance?.labelPosition === POSITION.Top) { | ||
| return result | ||
| } | ||
|
|
||
| const labelWidth = props.labelWidth || state.form.state.labelWidth | ||
| const labelWidth = props.labelWidth || state.formInstance?.state?.labelWidth |
There was a problem hiding this comment.
💡 Codebase verification
Migration from state.form to state.formInstance needs attention
The verification reveals inconsistency in the codebase:
packages/renderless/src/form-item/index.tsstill uses the oldstate.formpatternpackages/mobile/components/form-item/src/renderless/index.tshas been updated to usestate.formInstance
This suggests an incomplete migration where the renderless package hasn't been updated to match the mobile package's changes. The changes need to be synchronized across both packages to maintain consistency.
packages/renderless/src/form-item/index.ts: Needs to be updated to usestate.formInstancepatternpackages/mobile/components/form-item/src/renderless/index.ts: Changes are correct but depend on the renderless package
🔗 Analysis chain
LGTM! Verify form instance property access.
The optional chaining operators improve null safety when accessing formInstance properties.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify formInstance property access patterns across the codebase
# Look for any remaining instances of state.form that might have been missed
rg "state\.form\." --type ts
Length of output: 1444
Script:
#!/bin/bash
# Let's check if there are any other files in the codebase that use formInstance
rg "formInstance" --type ts -A 2
Length of output: 13525
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
Release Notes
titleproperty for input components, enhancing configuration options.