Vonage / vivid-react

Typescript friendly Reactjs :atom: wrappers/bindings for Vonage's web UI 🎨 toolbelt
https://www.npmjs.com/package/@vonage/vivid-react
5 stars 3 forks source link

VwcButtonToggleGroup values prop sometimes ignored #44

Closed GREsau closed 2 years ago

GREsau commented 2 years ago

To reproduce (full repro including package.json at https://github.com/newvoicemedia/graham-esau/tree/master/vivid-repro):

import { createRoot } from 'react-dom/client'
import { initVivid } from '@vonage/vivid-react'
import VwcButtonToggleGroup from '@vonage/vivid-react/VwcButtonToggleGroup'
import VwcButton from '@vonage/vivid-react/VwcButton'

const container = document.getElementById('app')!

initVivid(container, () => {
  const root = createRoot(container)
  root.render(
    <VwcButtonToggleGroup required values={['foo']}>
      <VwcButton value='foo'>Foo</VwcButton>
      <VwcButton value='bar'>Bar</VwcButton>
    </VwcButtonToggleGroup>
  )
})

On page load, the Foo button should be selected, but usually it is not.

I think this is due to a race condition where at the point the react wrapper sets the value property, the vwc-button-toggle-group's items property is empty. I confirmed this is the case by adding console.log([...this.children], this.items) to the vwc-button-toggle-group values setter. When Foo is correctly selected, it logs [vwc-button, vwc-button] [vwc-button, vwc-button] - but when it's not selected, it logs [vwc-button, vwc-button] [], showing that items is empty.

k-paxian commented 2 years ago

Indeed, sharp catch πŸŽ‰
That's a good one for @yinonov @vng-vivid

There is a bit cumbersome logic buried here and here and this line is not allowing setting an empty array as a value w/o immediate npe πŸ˜„ and this logic needs to be re written in order to support races between properties and children's

values property should allow idempotent usage, no matter when and how many times this property is set, the state should be consistent and independent from absence and/or presence of child buttons, since element is actually allows children mutations in runtime πŸ˜ƒ cite from here: One can add more buttons dynamically

While we all wait this issue is being fixed on vivid side, I could offer an immediate potential workaround on the wrappers side, with the latest wrappers package 2.10.22 you can try something like that:

    <VwcButtonToggleGroup
      ref={btnGroupElement =>
        setTimeout(() => {
          if (!btnGroupElement) {
            return
          }
          btnGroupElement.values = ['foo']
        }, 0)
      }
      required
    >
      <VwcButton value='foo'>Foo</VwcButton>
      <VwcButton value='bar'>Bar</VwcButton>
    </VwcButtonToggleGroup>

please let me know if this helps.

GREsau commented 2 years ago

Thanks, I just implemented a very similar workaround myself: https://github.com/newvoicemedia/webrtc-traffic-management-ui/blob/779130ef9058164e8254b83741056a025e5b8094/src/components/ButtonToggleGroup.tsx