db-ui / mono

DB UX Design System Monorepo - Provides Design Tokens and components for Web UIs
https://db-ui.github.io/mono/review/main/
Apache License 2.0
32 stars 6 forks source link

db-checkbox: checked property leads to disabled checkbox #1646

Closed annsch closed 6 months ago

annsch commented 8 months ago

Which generators are impacted?

Reproduction case

With use of checked={true} in react-components the ability to manually change <input type="checkbox">'s value by click get's lost. This bug can also be reproduced in patternhubs overview: https://db-ui.github.io/mono/review/main/components/checkbox/overview

Expected Behaviour

After setting checked={true}checkboxes should be still changeable

Screenshots

No response

Browser version

None

Add any other context about the problem here.

nmerget commented 8 months ago

That's basically the correct behavior.

If you set checked={true} to control the input you set a const which would not change. But you are right, we need to test the controlled component and defaultChecked in the e2e-tests

annsch commented 7 months ago

Ok, I get your point and from a react point of view you're right. But then we have to discuss if we shouldn't develop the native behaviour first and extend it with the react-way ... <input type="checkbox" checked> is still changeable

mfranzke commented 7 months ago

Ok, I get your point and from a react point of view you're right. But then we have to discuss if we shouldn't develop the native behaviour first and extend it with the react-way ... <input type="checkbox" checked> is still changeable

plus that not being able to control the value from the UI at all doesn't seem to be what controlled components are for, are they? Elsewhere, does it even also mean that I wouldn't be able to type any content into text input fields? That doesn't sound right to me.

nmerget commented 7 months ago

I don't get it...

Patternhub is a next(react) app so if we add checked={true} we have a component with a const which is always true. So you cannot change it event by click because we control the behavior. That's the reason why there is a defaultChecked/defaultValue in React. The default behaviour is just different from the synthetic DOM of React so the example you gave feels wrong, but that's how React works. To fix the example we need to add a state and fetch the onChange event to reflect the checked={checkedStateValue}