fix(ToggleGroup): making toggle pressed logic a computed property (#759)

* fix: making toggle pressed logic a computed property * fix: cant deselect single toggle * test: add more test toggle group --------- Co-authored-by: zernonia <zernonia@gmail.com>
radix-vue · Mar 18, 2024 · 8b0e49e · 8b0e49e
1 parent 382c746
commit 8b0e49e
Show file tree

Hide file tree

Showing 4 changed files with 137 additions and 28 deletions.
diff --git a/packages/radix-vue/src/ToggleGroup/ToggleGroup.test.ts b/packages/radix-vue/src/ToggleGroup/ToggleGroup.test.ts
@@ -3,13 +3,118 @@ import ToggleGroup from './story/_ToggleGroup.vue'
 import type { DOMWrapper, VueWrapper } from '@vue/test-utils'
 import { mount } from '@vue/test-utils'
 import { axe } from 'vitest-axe'
+import { nextTick, ref } from 'vue'
 
 describe('given default Toggle Group', () => {
   let wrapper: VueWrapper<InstanceType<typeof ToggleGroup>>
   let triggers: DOMWrapper<HTMLButtonElement>[]
 
   beforeEach(() => {
-    wrapper = mount(ToggleGroup, { attachTo: document.body })
+    wrapper = mount(ToggleGroup, {
+      attachTo: document.body,
+      props: {
+        defaultValue: 'center',
+      },
+    })
+    triggers = wrapper.findAll('button')
+  })
+
+  it('should pass axe accessibility tests', async () => {
+    expect(await axe(wrapper.element)).toHaveNoViolations()
+  })
+
+  it('should have active toggle=center', () => {
+    expect(triggers[0].attributes('data-state')).toBe('off')
+    expect(triggers[1].attributes('data-state')).toBe('on')
+    expect(triggers[2].attributes('data-state')).toBe('off')
+  })
+
+  describe('after toggling current active', () => {
+    beforeEach(async () => {
+      triggers[1].element.focus()
+      triggers[1].element.click()
+      await nextTick()
+    })
+
+    it('should deselect pre-existing value', () => {
+      expect(triggers[0].attributes('data-state')).toBe('off')
+      expect(triggers[1].attributes('data-state')).toBe('off')
+      expect(triggers[2].attributes('data-state')).toBe('off')
+    })
+  })
+
+  describe('after triggering ArrowRight', () => {
+    beforeEach(async () => {
+      triggers[1].element.focus()
+      await triggers[1].trigger('keydown', { key: 'ArrowRight' })
+    })
+
+    it('should received focus for the next toggle', () => {
+      expect(triggers[2].element).toBe(document.activeElement)
+    })
+
+    describe('after toggling', () => {
+      beforeEach(() => {
+        triggers[2].element.click()
+      })
+
+      it('should have next active value', () => {
+        expect(triggers[0].attributes('data-state')).toBe('off')
+        expect(triggers[1].attributes('data-state')).toBe('off')
+        expect(triggers[2].attributes('data-state')).toBe('on')
+      })
+
+      describe('after triggering ArrowRight again', () => {
+        beforeEach(async () => {
+          await triggers[2].trigger('keydown', { key: 'ArrowRight' })
+        })
+
+        it('should received focus for the first toggle', () => {
+          expect(triggers[0].element).toBe(document.activeElement)
+        })
+      })
+    })
+  })
+
+  describe('after triggering ArrowLeft', () => {
+    beforeEach(async () => {
+      triggers[1].element.focus()
+      await triggers[1].trigger('keydown', { key: 'ArrowLeft' })
+    })
+
+    it('should received focus for the next toggle', () => {
+      expect(triggers[0].element).toBe(document.activeElement)
+    })
+
+    describe('after toggling', () => {
+      beforeEach(() => {
+        triggers[0].element.click()
+      })
+
+      it('should have next active value', () => {
+        expect(triggers[0].attributes('data-state')).toBe('on')
+        expect(triggers[1].attributes('data-state')).toBe('off')
+        expect(triggers[2].attributes('data-state')).toBe('off')
+      })
+    })
+  })
+})
+
+describe('given multiple value Toggle Group', () => {
+  let wrapper: VueWrapper<InstanceType<typeof ToggleGroup>>
+  let triggers: DOMWrapper<HTMLButtonElement>[]
+
+  const modelValue = ref(['center', 'right'])
+  beforeEach(() => {
+    wrapper = mount(ToggleGroup, {
+      attachTo: document.body,
+      props: {
+        'modelValue': modelValue.value,
+        // @ts-expect-error ignore
+        'onUpdate:modelValue': ev => modelValue.value = ev,
+        'type': 'multiple',
+      },
+    })
     triggers = wrapper.findAll('button')
   })
 
@@ -18,9 +123,9 @@ describe('given default Toggle Group', () => {
   })
 
   it('should have active toggle=center', () => {
-    expect(triggers[0].attributes('data-active')).toBe('false')
-    expect(triggers[1].attributes('data-active')).toBe('true')
-    expect(triggers[2].attributes('data-active')).toBe('false')
+    expect(triggers[0].attributes('data-state')).toBe('off')
+    expect(triggers[1].attributes('data-state')).toBe('on')
+    expect(triggers[2].attributes('data-state')).toBe('on')
   })
 
   describe('after triggering ArrowRight', () => {
@@ -39,9 +144,9 @@ describe('given default Toggle Group', () => {
       })
 
       it('should have next active value', () => {
-        expect(triggers[0].attributes('data-active')).toBe('false')
-        expect(triggers[1].attributes('data-active')).toBe('false')
-        expect(triggers[2].attributes('data-active')).toBe('true')
+        expect(triggers[0].attributes('data-state')).toBe('off')
+        expect(triggers[1].attributes('data-state')).toBe('on')
+        expect(triggers[2].attributes('data-state')).toBe('off')
       })
 
       describe('after triggering ArrowRight again', () => {
@@ -72,9 +177,9 @@ describe('given default Toggle Group', () => {
       })
 
       it('should have next active value', () => {
-        expect(triggers[0].attributes('data-active')).toBe('true')
-        expect(triggers[1].attributes('data-active')).toBe('false')
-        expect(triggers[2].attributes('data-active')).toBe('false')
+        expect(triggers[0].attributes('data-state')).toBe('on')
+        expect(triggers[1].attributes('data-state')).toBe('on')
+        expect(triggers[2].attributes('data-state')).toBe('on')
       })
     })
   })

diff --git a/packages/radix-vue/src/ToggleGroup/ToggleGroupItem.vue b/packages/radix-vue/src/ToggleGroup/ToggleGroupItem.vue
@@ -25,6 +25,12 @@ const rootContext = injectToggleGroupRootContext()
 const disabled = computed(() => rootContext.disabled?.value || props.disabled)
 const pressed = computed(() => rootContext.modelValue.value?.includes(props.value))
 
+const isPressed = computed(() => {
+  return rootContext.isSingle.value
+    ? rootContext.modelValue.value === props.value
+    : rootContext.modelValue.value?.includes(props.value)
+})
+
 const { forwardRef } = useForwardExpose()
 </script>
 
@@ -39,11 +45,7 @@ const { forwardRef } = useForwardExpose()
       v-bind="props"
       :ref="forwardRef"
       :disabled="disabled"
-      :pressed="
-        rootContext.isSingle
-          ? rootContext.modelValue.value === value
-          : rootContext.modelValue.value?.includes(value)
-      "
+      :pressed="isPressed"
       @update:pressed="rootContext.changeModelValue(value)"
     >
       <slot />

diff --git a/packages/radix-vue/src/ToggleGroup/story/_ToggleGroup.vue b/packages/radix-vue/src/ToggleGroup/story/_ToggleGroup.vue
@@ -1,16 +1,20 @@
 <script setup lang="ts">
 import { Icon } from '@iconify/vue'
-import { ref } from 'vue'
+import type { ToggleGroupRootEmits, ToggleGroupRootProps } from '../'
 import { ToggleGroupItem, ToggleGroupRoot } from '../'
+import { useForwardPropsEmits } from '@/shared'
 
-const toggleStateSingle = ref('center')
+const props = defineProps<ToggleGroupRootProps>()
+const emits = defineEmits<ToggleGroupRootEmits>()
+
+const forwarded = useForwardPropsEmits(props, emits)
 
 const toggleGroupItemClasses
   = 'hover:bg-violet3 color-mauve11 data-[state=on]:bg-violet6 data-[state=on]:text-violet12 flex h-[35px] w-[35px] items-center justify-center bg-white text-base leading-4 first:rounded-l last:rounded-r focus:z-10 focus:shadow-[0_0_0_2px] focus:shadow-black focus:outline-none'
 </script>
 
 <template>
-  <ToggleGroupRoot v-model="toggleStateSingle" class="flex">
+  <ToggleGroupRoot v-bind="forwarded" class="flex">
     <ToggleGroupItem
       value="left"
       aria-label="Toggle italic"

diff --git a/packages/radix-vue/src/shared/useSingleOrMultipleValue.ts b/packages/radix-vue/src/shared/useSingleOrMultipleValue.ts
@@ -13,13 +13,14 @@ import type { SingleOrMultipleProps } from './types'
  * 4. Return 'multiple' if modelValue is an array, else return 'single'.
  */
 function validateProps({ type, defaultValue, modelValue }: SingleOrMultipleProps) {
+  const value = modelValue || defaultValue
   // One of the three must be defined
   if (!type && !modelValue && !defaultValue)
     throw new Error('Either the `type` or the `value` or `default-value` prop must be defined.')
 
   if (modelValue !== undefined && defaultValue !== undefined && typeof modelValue !== typeof defaultValue) {
     throw new Error(
-        `Invalid prop \`value\` of value \`${modelValue}\` supplied to \`AccordionRoot\`, should be the same type as the \`defaultValue\` prop, which is \`${defaultValue}\`. The \`value\` prop must be:
+        `Invalid prop \`value\` of value \`${modelValue}\` supplied, should be the same type as the \`defaultValue\` prop, which is \`${defaultValue}\`. The \`value\` prop must be:
   ${type === 'single' ? '- a string' : type === 'multiple' ? '- an array of strings' : '- a string\n- an array of strings'}
   - \`undefined\``)
   }
@@ -31,19 +32,19 @@ function validateProps({ type, defaultValue, modelValue }: SingleOrMultipleProps
     const propUsed = modelValue !== undefined ? 'modelValue' : 'defaultValue'
     const typeUsed = propUsed === 'modelValue' ? typeof modelValue : typeof defaultValue
     if (type === 'single' && isArray) {
-      console.error(`Invalid prop \`${propUsed}\` of type ${typeUsed} supplied to \`AccordionRoot\` with type \`single\`. The \`modelValue\` prop must be a string or \`undefined\`.
+      console.error(`Invalid prop \`${propUsed}\` of type ${typeUsed} supplied with type \`single\`. The \`modelValue\` prop must be a string or \`undefined\`.
 You can remove the \`type\` prop to let the component infer the type from the ${propUsed} prop.`)
       return 'multiple'
     }
     else if (type === 'multiple' && !isArray) {
-      console.error(`Invalid prop \`${propUsed}\` of type ${typeUsed} supplied to \`AccordionRoot\` with type \`multiple\`. The \`modelValue\` prop must be an array of strings or \`undefined\`.
+      console.error(`Invalid prop \`${propUsed}\` of type ${typeUsed} supplied with type \`multiple\`. The \`modelValue\` prop must be an array of strings or \`undefined\`.
     You can remove the \`type\` prop to let the component infer the type from the ${propUsed} prop.`)
       return 'single'
     }
   }
 
   if (canTypeBeInferred)
-    return Array.isArray(modelValue) ? 'multiple' : 'single'
+    return Array.isArray(value) ? 'multiple' : 'single'
   else
     return type
 }
@@ -55,13 +56,10 @@ function getDefaultType({ type, defaultValue, modelValue }: SingleOrMultipleProp
   return validateProps({ type, defaultValue, modelValue })
 }
 
-function getDefaultValue({ type, defaultValue }: SingleOrMultipleProps, modelValue: undefined | string | string[]) {
+function getDefaultValue({ type, defaultValue }: SingleOrMultipleProps) {
   if (defaultValue !== undefined)
     return defaultValue
 
-  if (modelValue !== undefined)
-    return modelValue
-
   return (type === 'single') ? undefined : []
 }
 
@@ -71,12 +69,12 @@ export function useSingleOrMultipleValue<P extends SingleOrMultipleProps, Name e
 ) {
   const type = ref(getDefaultType(props))
   const modelValue = useVModel(props, 'modelValue', emits, {
-    defaultValue: getDefaultValue(props, props.modelValue),
+    defaultValue: getDefaultValue(props),
     passive: (props.modelValue === undefined) as false,
   }) as Ref<string | string[] | undefined>
 
   watch(
-    () => [props.type, props.modelValue],
+    () => [props.type, props.modelValue, props.defaultValue],
     () => {
       const validatedType = validateProps(props)
       if (type.value !== validatedType)