feat(properties): adds validations and enhancements to property forms

This change will reset any invalid values entered into the PropertyForm dialog. This also makes a major accessibility improvement to the Item properties dialog.
jshor · Apr 2, 2024 · 3704f89 · 3704f89
1 parent 1847b28
commit 3704f89
Show file tree

Hide file tree

Showing 11 changed files with 296 additions and 167 deletions.
diff --git a/src/components/PropertyForm.vue b/src/components/PropertyForm.vue
@@ -1,7 +1,7 @@
 <template>
   <div class="property-form">
     <div
-      v-for="(data, propertyName) in model"
+      v-for="(data, propertyName) in (model as Required<PropertyList>)"
       :key="propertyName"
       class="property-form__entry"
     >
@@ -39,16 +39,16 @@
         class="property-form__switch"
       >
         <property-switch
-          v-model.boolean="data.value"
+          v-model.boolean="(data.value as boolean)"
           :id="`${id}_${propertyName}`"
           :disabled="data.disabled"
           @update:modelValue="onSwitch(propertyName)"
         />
       </div>
 
-      <!-- text boxes -->
+      <!-- numeric input -->
       <input
-        v-else
+        v-else-if="data.type === 'number'"
         v-model="data.value"
         :min="data.min"
         :max="data.max"
@@ -59,6 +59,17 @@
         @change="onChange"
       />
 
+      <!-- text boxes -->
+      <input
+        v-else
+        v-model="data.value"
+        :type="data.type"
+        :id="`${id}_${propertyName}`"
+        :disabled="data.disabled"
+        class="property-form__input"
+        @change="onChange"
+      />
+
       <small
         v-if="data.description"
         class="property-form__description"
@@ -75,7 +86,9 @@ import cloneDeep from 'lodash.clonedeep'
 import PropertySwitch from '@/components/properties/PropertySwitch.vue'
 import Icon from '@/components/Icon.vue'
 import isMobile from '@/utils/isMobile'
-import ItemProperties from '@/types/interfaces/ItemProperties'
+import Property from '@/types/interfaces/Property'
+
+type PropertyList = Record<string, Property<string | boolean | number>>
 
 export default defineComponent({
   name: 'PropertyForm',
@@ -85,7 +98,7 @@ export default defineComponent({
   },
   props: {
     modelValue: {
-      type: Object as PropType<ItemProperties>,
+      type: Object as PropType<PropertyList>,
       default: () => ({})
     },
     id: {
@@ -94,14 +107,14 @@ export default defineComponent({
     }
   },
   setup (props, { emit }) {
-    const getModel = (model: ItemProperties) => {
-      const properties: ItemProperties = {}
+    const getModel = (model: PropertyList) => {
+      const properties: PropertyList = {}
 
       for (const key in model) {
-        const k = key as keyof ItemProperties
+        const k = key as keyof PropertyList
         const isVisible = isMobile
-          ? !model[k].desktopOnly
-          : !model[k].mobileOnly
+          ? !model[k]?.desktopOnly
+          : !model[k]?.mobileOnly
 
         if (isVisible) {
           properties[k] = cloneDeep(model[k])
@@ -110,16 +123,17 @@ export default defineComponent({
 
       return properties
     }
-    const model = ref<ItemProperties>(getModel(props.modelValue))
+    const model = ref<PropertyList>(getModel(props.modelValue))
+    const errors = ref<string[]>([])
 
     watch(() => props.modelValue, value => {
       model.value = getModel(value)
     }, { deep: true })
 
-    function onSwitch (propertyName: keyof ItemProperties) {
+    function onSwitch (propertyName: keyof PropertyList) {
       const property = model.value[propertyName]
 
-      property.excludes?.forEach((p: keyof ItemProperties) => {
+      property?.excludes?.forEach((p: keyof PropertyList) => {
         if (property.value && model.value[p].type === 'boolean') {
           model.value[p].value = false
         }
@@ -129,11 +143,36 @@ export default defineComponent({
     }
 
     function onChange () {
+      validate()
       emit('update:modelValue', model.value)
     }
 
+    function validate () {
+      for (const key in model.value) {
+        const k = key as keyof PropertyList
+
+        if (!isValidValue(model.value[k])) {
+          model.value[k].value = props.modelValue[k].value
+        }
+      }
+    }
+
+    function isValidValue (property: Property<string | boolean | number>) {
+      if (property.type === 'boolean') return true
+      if (property.type === 'number') {
+        if (typeof property.value !== 'number') return false
+        if (property.max !== undefined && property.max < property.value) return false
+        if (property.min !== undefined && property.min > property.value) return false
+
+        return true
+      }
+
+      return !!property.value
+    }
+
     return {
       model,
+      errors,
       close,
       onSwitch,
       onChange
@@ -177,8 +216,8 @@ export default defineComponent({
     color: var(--color-primary);
     font-family: system-ui, sans-serif;
     padding: 0.25em;
-      width: auto;
-      height: auto;
+    width: auto;
+    height: auto;
 
     &[type="color"] {
       padding: 0;

diff --git a/src/components/basic/WireDraggable.vue b/src/components/basic/WireDraggable.vue
@@ -39,6 +39,9 @@
     />
     <path
       class="wire__clickable"
+      :class="{
+        'wire__clickable--mobile': isMobile
+      }"
       data-test="wire-clickable"
       fill="none"
       ref="wireRef"
@@ -47,7 +50,7 @@
       @mousedown.left="onLeftMouseDown"
       @mouseup="onMouseUp"
       @touchstart="onTouchStart"
-      @touchmove="onTouchMove"
+      @touchmove="onTouchDrag"
       @touchend="onTouchEnd"
       @touchcancel="dragEnd"
     />
@@ -60,6 +63,8 @@ import LogicValue from '../../types/enums/LogicValue'
 import WireGeometry from '../../types/types/WireGeometry'
 import { useDraggable } from '../../composables/useDraggable'
 import Point from '../../types/interfaces/Point'
+import isMobile from '../../utils/isMobile'
+import { TOUCH_SHORT_HOLD_TIMEOUT } from '@/constants'
 
 export default defineComponent({
   name: 'WireDraggable',
@@ -132,16 +137,16 @@ export default defineComponent({
 
       props.isSelected
         ? emit('deselect')
-        : emit('select', true)
+        : emit('select', false)
     }
 
     /**
      * Drag start event handler.
      */
     function onDragStart (position: Point, offset: Point) {
-      // if (!isTouchDragging.value) {
-      //   emit('add', position, offset)
-      // }
+      if (!isTouchDragging.value) {
+        emit('add', position, offset)
+      }
     }
 
     function onDragEnd () {
@@ -155,11 +160,11 @@ export default defineComponent({
      * The draggable `mousedown` event will continue to be propagated.
      */
     function onLeftMouseDown ($event: MouseEvent) {
-      // onMouseDown($event)
+      onMouseDown($event)
 
-      // if (!props.isSelected) {
-      //   emit('select', !$event.ctrlKey)
-      // }
+      if (!props.isSelected) {
+        emit('select', !$event.ctrlKey)
+      }
     }
 
     const allowTouchDrag = computed(() => isTouchDragging.value)
@@ -172,20 +177,29 @@ export default defineComponent({
       dragEnd,
     } = useDraggable({
       allowTouchDrag,
-      longTouchTimeout: 500, // TODO: const
+      longTouchTimeout: TOUCH_SHORT_HOLD_TIMEOUT,
       onTouched,
       onDragStart,
       onDrag: (p: Point, o: Point) => emit('move', p, o),
       onDragEnd: () => onDragEnd()
     })
 
+    function onTouchDrag ($event: TouchEvent) {
+      if (isTouchDragging.value) {
+        $event.stopPropagation()
+      }
+
+      onTouchMove($event)
+    }
+
     return {
+      isMobile,
       wireRef,
       LogicValue,
       onLeftMouseDown,
       onMouseUp,
       onTouchStart,
-      onTouchMove,
+      onTouchDrag,
       onTouchEnd,
       dragEnd
     }
@@ -207,9 +221,13 @@ export default defineComponent({
 
   &__clickable {
     animation: none;
-    stroke-width: 16;
     pointer-events: visibleStroke;
+    stroke-width: 16;
     cursor: pointer;
+
+    &--mobile {
+      stroke-width: 24;
+    }
   }
 
   &__outline {

diff --git a/src/components/basic/__tests__/WireDraggable.spec.ts b/src/components/basic/__tests__/WireDraggable.spec.ts
@@ -3,6 +3,7 @@ import WireDraggable from '../WireDraggable.vue'
 import { createControlPoint } from '@/store/document/actions/__tests__/__helpers__'
 import boundaries from '@/store/document/geometry/boundaries'
 import renderLayout from '@/store/document/geometry/wire'
+import { TOUCH_SHORT_HOLD_TIMEOUT } from '@/constants'
 
 describe('Draggable wire component', () => {
   let wrapper: VueWrapper<typeof WireDraggable>
@@ -26,7 +27,7 @@ describe('Draggable wire component', () => {
   describe('when using touch gestures', () => {
     const getTouchEvent = (clientX: number, clientY: number): Touch => ({ clientX, clientY }) as Touch
 
-    it('should emit `add` to add a control point after being held down in the same place for more than 500ms', async () => {
+    it('should emit `add` to add a control point after being held down in the same place for more than the short touch-hold timeout', async () => {
       jest.useFakeTimers()
 
       await wrapper
@@ -35,7 +36,7 @@ describe('Draggable wire component', () => {
           touches: [new TouchEvent('touchstart')]
         })
 
-      jest.advanceTimersByTime(500)
+      jest.advanceTimersByTime(TOUCH_SHORT_HOLD_TIMEOUT + 1)
 
       expect(wrapper.emitted()).toHaveProperty('add')
     })
@@ -71,7 +72,7 @@ describe('Draggable wire component', () => {
             touches: [getTouchEvent(x, y)]
           })
 
-        jest.advanceTimersByTime(500)
+        jest.advanceTimersByTime(TOUCH_SHORT_HOLD_TIMEOUT + 1)
 
         expect(wrapper.emitted()).not.toHaveProperty('add')
       })
@@ -87,7 +88,7 @@ describe('Draggable wire component', () => {
             touches: [getTouchEvent(x, y)]
           })
 
-        jest.advanceTimersByTime(500)
+        jest.advanceTimersByTime(TOUCH_SHORT_HOLD_TIMEOUT + 1)
 
         expect(wrapper.emitted()).toHaveProperty('add')
       })

diff --git a/src/components/interactive/Draggable.vue b/src/components/interactive/Draggable.vue
@@ -25,7 +25,7 @@
 <script lang="ts">
 import Point from '@/types/interfaces/Point'
 import { defineComponent, PropType, computed, ref, nextTick } from 'vue'
-import { TOUCH_HOLD_TIMEOUT } from '@/constants'
+import { TOUCH_LONG_HOLD_TIMEOUT } from '@/constants'
 import { useDraggable } from '@/composables/useDraggable'
 
 export default defineComponent({
@@ -141,7 +141,7 @@ export default defineComponent({
       dragEnd,
     } = useDraggable({
       allowTouchDrag,
-      longTouchTimeout: TOUCH_HOLD_TIMEOUT,
+      longTouchTimeout: TOUCH_LONG_HOLD_TIMEOUT,
       onTouched,
       onDragStart: (p: Point, o: Point) => emit('dragStart', p, o),
       onDrag: (p: Point, o: Point) => emit('drag', p, o),

diff --git a/src/components/interactive/__tests__/Draggable.spec.ts b/src/components/interactive/__tests__/Draggable.spec.ts
@@ -2,6 +2,7 @@ import boundaries from '@/store/document/geometry/boundaries'
 import { mount, VueWrapper } from '@vue/test-utils'
 import { ComponentPublicInstance } from 'vue'
 import Draggable from '../Draggable.vue'
+import { TOUCH_LONG_HOLD_TIMEOUT } from '@/constants'
 
 describe('Draggable component', () => {
   let wrapper: VueWrapper
@@ -171,14 +172,14 @@ describe('Draggable component', () => {
   describe('when using touch gestures', () => {
     const getTouchEvent = (clientX: number, clientY: number): Touch => ({ clientX, clientY }) as Touch
 
-    it('should emit `touchhold` after being held down in the same place for more than 500ms', async () => {
+    it('should emit `touchhold` after being held down in the same place for more than the long touch-hold timeout', async () => {
       jest.useFakeTimers()
 
       await wrapper.trigger('touchstart', {
         touches: [new TouchEvent('touchstart')]
       })
 
-      jest.advanceTimersByTime(500)
+      jest.advanceTimersByTime(TOUCH_LONG_HOLD_TIMEOUT + 1)
 
       expect(wrapper.emitted()).toHaveProperty('touchhold')
     })
@@ -210,7 +211,7 @@ describe('Draggable component', () => {
           touches: [getTouchEvent(x, y)]
         })
 
-        jest.advanceTimersByTime(500)
+        jest.advanceTimersByTime(TOUCH_LONG_HOLD_TIMEOUT + 1)
 
         expect(wrapper.emitted()).not.toHaveProperty('touchhold')
       })
@@ -224,7 +225,7 @@ describe('Draggable component', () => {
           touches: [getTouchEvent(x, y)]
         })
 
-        jest.advanceTimersByTime(500)
+        jest.advanceTimersByTime(TOUCH_LONG_HOLD_TIMEOUT + 1)
 
         expect(wrapper.emitted()).toHaveProperty('touchhold')
       })