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.

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(form): modify mobile form problem #2643

Merged
merged 4 commits into from
Dec 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
123 changes: 18 additions & 105 deletions packages/mobile/components/form-item/src/mobile.vue
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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?

import { renderless, api } from './renderless/vue'
import LabelWrap from './label-wrap'
import Tooltip from '../../tooltip'
Expand Down Expand Up @@ -49,14 +49,14 @@ export default defineComponent({
handleMouseleave
} = this as unknown as IFormItemInstance

const { validateIcon, isErrorInline, isErrorBlock, tooltipType } = state
const isMobile = state.mode === 'mobile'
const { validateIcon, isErrorInline, isErrorBlock, formInstance } = state
const isMobile = true
Copy link

Choose a reason for hiding this comment

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

🛠️ 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.

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
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
const isShowError = state.validateState === 'error' && showMessage && formInstance.showMessage
const isShowError = state.validateState === 'error' && showMessage && formInstance?.showMessage

const validateTag = state.formInstance && state.formInstance.validateTag
let validateMessage

Expand Down Expand Up @@ -104,100 +104,15 @@ export default defineComponent({
: null

const getFormItemContnet = () => {
if (isMobile) {
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
return ItemContent
}

const tooltipTriggerContent =
ItemContent.length > 1
? h('div', { class: 'tiny-form-item__content-muti-children' }, ItemContent)
: ItemContent[0]

if (!this.showMessage) {
return tooltipTriggerContent
}

const formAppendToBody = state.formInstance && state.formInstance.appendToBody
const appendToBody =
typeof this.appendToBody === 'boolean'
? this.appendToBody
: typeof formAppendToBody === 'boolean'
? formAppendToBody
: true
const validatePosition =
this.validatePosition || (state.formInstance && state.formInstance.validatePosition) || 'top-end'

const popperOptions = {
...state.formInstance.popperOptions,
...this.popperOptions,
forceAbsolute: !appendToBody,
onUpdate: (options) => {
const popper = options.instance._popper
const translate3d = popper.style.transform
const matchTranslate = translate3d.match(/translate3d\((\w+)px, (\w+)px, (\w+)px\)/)

if (!Array.isArray(matchTranslate)) {
return
}

const [x, y, z] = matchTranslate.slice(1)

popper.style.transform = `translate3d(${x}px, ${parseInt(y, 10)}px, ${z}px)`
}
}
const validateIconNode = validateIcon ? h(validateIcon, { class: 'tooltip-validate-icon' }) : null
return h(
'tooltip',
{
props: {
popperClass: `${classPrefix}form__valid`,
arrowOffset: 0,
adjustArrow: true,
type: tooltipType,
disabled: state.getValidateType !== 'tip',
placement: validatePosition,
manual: true,
appendToBody,
popperOptions,
modelValue: isShowError ? state.canShowTip : false,
zIndex: 'relative',
renderContent() {
let tooltipContent
if (errorSlot) {
tooltipContent = [errorSlot]
} else {
tooltipContent = [
validateIconNode,
<span class={`${classPrefix}form-item__validate-message`}>{state.validateMessage}</span>
]
}

return tooltipContent
}
},
on: {
'update:modelValue': (value) => {
state.canShowTip = value
}
},
ref: 'tooltip'
},
[
!isVue2 && tooltipTriggerContent.type === hooks.Text ? (
<span>{tooltipTriggerContent}</span>
) : (
tooltipTriggerContent
)
]
)
const validatePosition = this.validatePosition || state.formInstance.validatePosition || 'right'
Copy link

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
const validatePosition = this.validatePosition || state.formInstance.validatePosition || 'right'
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
Comment on lines +107 to +114
Copy link

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
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

return ItemContent
}

const FormItemContnet = ItemContent ? getFormItemContnet() : null
Expand Down Expand Up @@ -227,7 +142,7 @@ export default defineComponent({
{
props: {
isAutoWidth: state.labelStyle && state.labelStyle.width === 'auto',
updateAll: state.form.labelWidth === 'auto',
updateAll: state.formInstance?.labelWidth === 'auto',
isMobile: state.mode === 'mobile'
}
},
Expand All @@ -249,7 +164,7 @@ export default defineComponent({
mouseleave: handleMouseleave
}
},
labelSlot || label + state.form.labelSuffix
labelSlot || label + (state.formInstance?.labelSuffix || '')
)
: null
]
Expand Down Expand Up @@ -283,7 +198,6 @@ export default defineComponent({
}
},
[
!isMobile ? LabelContent : null,
h(
'div',
{
Expand All @@ -294,9 +208,9 @@ export default defineComponent({
style: !isMobile && state.contentStyle
},
[
isMobile ? LabelContent : null,
LabelContent,
FormItemContnet,
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
FormItemContnet,
FormItemContent,

isMobile ? validateMessage : null,
validateMessage,
h(
'transition',
{
Expand All @@ -305,8 +219,7 @@ export default defineComponent({
}
},
[ErrorContent]
),
isMobile ? null : ExtraTip
)
]
)
]
Expand Down
51 changes: 15 additions & 36 deletions packages/mobile/components/form-item/src/renderless/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import { isNull } from '@opentiny/utils/type'
import debounce from '@opentiny/utils/deps/debounce'
import type {
IFormItemRenderlessParams,
IFormInstance,
IFormItemDisplayedValueParam,
IFormItemRule,
IFormItemTrigger,
Expand Down Expand Up @@ -80,11 +79,11 @@ export const computedLabelStyle =
(): IFormItemLabelStyle => {
const result = { width: '' }

if (state.form?.labelPosition === POSITION.Top) {
if (state.formInstance?.labelPosition === POSITION.Top) {
return result
}

const labelWidth = props.labelWidth || state.form.state.labelWidth
const labelWidth = props.labelWidth || state.formInstance?.state?.labelWidth
Comment on lines +82 to +86
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Migration from state.form to state.formInstance needs attention

The verification reveals inconsistency in the codebase:

  • packages/renderless/src/form-item/index.ts still uses the old state.form pattern
  • packages/mobile/components/form-item/src/renderless/index.ts has been updated to use state.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 use state.formInstance pattern
  • packages/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


if (labelWidth) {
result.width = labelWidth
Expand All @@ -98,12 +97,12 @@ export const computedValueStyle =
(): { width: string } => {
const result = { width: '' }

if (state.form?.labelPosition === POSITION.Top) {
if (state.formInstance?.labelPosition === POSITION.Top) {
result.width = '100%'
return result
}

const labelWidth = props.labelWidth || state.form.state.labelWidth
const labelWidth = props.labelWidth || state.formInstance?.state?.labelWidth

if (labelWidth) {
if (labelWidth === 'auto') {
Expand All @@ -122,20 +121,20 @@ export const computedContentStyle =
const result: StyleValue = {}
const label = props.label

if (state.form.labelPosition === POSITION.Top || state.form.inline) {
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') {
Comment on lines +124 to +137
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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') {

result.marginLeft = state.formInstance.state.autoLabelWidth
}
} else {
Expand All @@ -145,25 +144,6 @@ export const computedContentStyle =
return result
}

export const computedForm =
({ constants, vm, state }: Pick<IFormItemRenderlessParams, 'constants' | 'vm' | 'state'>) =>
(): IFormInstance | null => {
const { FORM_NAME, FORM_ITEM_NAME } = constants
let parent = vm.$parent?.$parent as IFormInstance | null
let parentName = parent?.$options?.componentName

while (parent && parentName !== FORM_NAME) {
if (parentName === FORM_ITEM_NAME) {
state.isNested = true
}

parent = parent?.$parent as IFormInstance | null
parentName = parent?.$options?.componentName
}

return parent
}

export const computedIsRequired =
({ api, state }: Pick<IFormItemRenderlessParams, 'api' | 'state'>) =>
(): boolean => {
Expand Down Expand Up @@ -231,7 +211,7 @@ export const getPropByPath = (obj: object, path: string, strict?: boolean) => {
export const computedFieldValue =
({ props, state }: Pick<IFormItemRenderlessParams, 'props' | 'state'>) =>
() => {
const model = state.form.model
const model = state.formInstance?.model

if (!model || !props.prop) {
return
Expand Down Expand Up @@ -287,7 +267,6 @@ export const validate =
state.validateState = VALIDATE_STATE.Validating

const descriptor = {}

if (rules && rules.length > 0) {
rules.forEach((rule) => {
delete rule.trigger
Expand Down Expand Up @@ -338,7 +317,7 @@ export const resetField =
state.validateState = ''
state.validateMessage = ''

let model = state.form.model || {}
let model = state.formInstance.model || {}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
let model = state.formInstance.model || {}
let model = state.formInstance?.model || {}

let value = state.fieldValue
let path = props.prop || ''

Expand Down Expand Up @@ -372,7 +351,7 @@ export const resetField =
export const getRules =
({ props, state }: Pick<IFormItemRenderlessParams, 'props' | 'state'>) =>
(): IFormItemRule[] => {
let formRules = state.form?.rules || {}
let formRules = state.formInstance?.rules || {}
const selfRules = props.rules as IFormItemRule[]
const requiredRule = props.required !== undefined ? { required: Boolean(props.required) } : []
const prop = getPropByPath(formRules, props.prop || '')
Expand Down Expand Up @@ -507,7 +486,7 @@ export const wrapValidate = ({
export const handleMouseenter =
({ state }: Pick<IFormItemRenderlessParams, 'state'>) =>
(e): void => {
if (!state.isDisplayOnly || !state.typeName || !state.form) return
if (!state.isDisplayOnly || !state.typeName || !state.formInstance) return
const dom = e.target
const text = dom.textContent
const font = window.getComputedStyle(dom).font
Expand All @@ -524,22 +503,22 @@ export const handleMouseenter =
}

if (res.o || overHeight) {
state.form.showTooltip(dom, state.displayedValue)
state.formInstance.showTooltip(dom, state.displayedValue)
}
}

export const handleLabelMouseenter =
({ props, state, slots }) =>
(e) => {
if (!state.form.overflowTitle || !state.form || slots.label) return
if (!state.formInstance?.overflowTitle || !state.formInstance || slots.label) return
const label = e.target
if (label && label.scrollWidth > label.offsetWidth) {
state.form.showTooltip(label, props.label + state.form.labelSuffix)
state.formInstance.showTooltip(label, props.label + state.formInstance.labelSuffix)
}
}

export const handleMouseleave = (state: IFormItemRenderlessParams['state']) => (): void => {
state.form && state.form.hideTooltip()
state.formInstance && state.formInstance.hideTooltip()
}

export const getDisplayedValue =
Expand Down
7 changes: 2 additions & 5 deletions packages/mobile/components/form-item/src/renderless/vue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ import {
computedLabelStyle,
computedValueStyle,
computedContentStyle,
computedForm,
computedIsRequired,
computedFieldValue,
computedGetValidateType,
Expand Down Expand Up @@ -102,15 +101,14 @@ const initState = ({
labelStyle: computed(() => api.computedLabelStyle()),
valueStyle: computed(() => api.computedValueStyle()),
contentStyle: computed(() => api.computedContentStyle()),
form: computed(() => api.computedForm() as IFormInstance),
fieldValue: computed(() => api.computedFieldValue()),
isRequired: computed(() => api.computedIsRequired()),
formInline: computed(() => state.formInstance.inline),
formSize: computed(() => state.formInstance.size),
formItemSize: computed(() => props.size || state.formSize),
isDisplayOnly: computed(() => state.formInstance.displayOnly),
labelPosition: computed(() => state.formInstance.labelPosition),
hideRequiredAsterisk: computed(() => state.formInstance.state.hideRequiredAsterisk),
hideRequiredAsterisk: computed(() => state.formInstance?.state?.hideRequiredAsterisk),
labelSuffix: computed(() => state.formInstance.labelSuffix),
labelWidth: computed(() => state.formInstance.labelWidth),
showMessage: computed(() => state.formInstance.showMessage),
Expand All @@ -120,7 +118,7 @@ const initState = ({
isErrorInline: computed(() => api.computedIsErrorInline()),
isErrorBlock: computed(() => api.computedIsErrorBlock()),
disabled: computed(() => state.formInstance.disabled),
tooltipType: computed(() => state.formInstance.state.tooltipType)
tooltipType: computed(() => state.formInstance?.state?.tooltipType)
})

return state
Expand All @@ -137,7 +135,6 @@ const initApi = ({ api, state, dispatch, broadcast, props, constants, vm, t, nex
computedLabelStyle: computedLabelStyle({ props, state }),
computedValueStyle: computedValueStyle({ props, state }),
computedContentStyle: computedContentStyle({ props, state }),
computedForm: computedForm({ constants, vm, state }),
computedFieldValue: computedFieldValue({ props, state }),
computedGetValidateType: computedGetValidateType({ props, state }),
computedValidateIcon: computedValidateIcon({ props, state }),
Expand Down
2 changes: 1 addition & 1 deletion packages/mobile/components/form/src/renderless/vue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ export const renderless = (

api.created()

provide('form', parent)
provide('form', vm)

provide('showAutoWidth', state.showAutoWidth)

Expand Down
Loading
Loading