cloudoperators / juno

Monorepo for the Juno modular frontend framework, apps, design system and component library
http://cloudoperators.github.io/juno/
Apache License 2.0
4 stars 0 forks source link

[TASK](ui): Migrate Switch component to TypeScript #255

Closed barsukov closed 31 minutes ago

barsukov commented 2 months ago

Task Description

We need to convert Switch and all related components from @cloudoperators/juno-ui-components to TypeScript

List of potential dependencies

Sub-tasks

Related Issues

Additional Context Please check this online codemode editor to change the PropTypes to a proper Ts interfaces

https://github.com/mskelton/ratchet Online editor: https://ratchet.mskelton.dev/

We need to be pretty concious about migration to ts cause it could create a potential breaking changes. Also if there will be too much changes at once maybe consider to split up the task again. Also some dependencies could be hidden and only discovered while executing the migration, could also lead to potential reconsidering the order of task execution.

guoda-puidokaite commented 3 days ago

Bug Found 🐞

Issue: Component can be valid and invalid at the same time, displaying icons for both states and applying the valid state to the switch outline. Explanation: See screenshot. Because there's two props called valid and invalid, the below bug is possible.

Screenshot 2024-11-05 at 10 32 17

Fix: We should combine valid and invalid into a single state to prevent redundancy and therefore, this bug. UX Improvements: See https://github.com/cloudoperators/juno/issues/255#issuecomment-2456596321

CC: @edda

guoda-puidokaite commented 3 days ago

Future Improvements 🚀

See screenshot:

Screenshot 2024-11-05 at 09 44 14
edda commented 3 days ago

Bug Found 🐞

Issue: Component can be valid and invalid at the same time, displaying icons for both states and applying the valid state to the switch outline. Explanation: See screenshot. Because there's two props called valid and invalid, the below bug is possible. Screenshot 2024-11-05 at 10 32 17 Fix: We should combine valid and invalid into a single state to prevent redundancy and therefore, this bug. UX Improvements: See #255 (comment)

CC: @edda

It was a conscious decision to separate the valid/invalid props. The reason is that it is not a boolean valid/invalid. invalid is for form validations (e.g. a mandatory field wasn't filled, a field was filled in the wrong format, etc.). The valid prop is not the inverse of invalid, i.e. if something ceases to be invalid (because the user corrected their input) you're not meant to switch to the valid state, instead you simply deactivate the invalid state. The valid state is only meant to be used for very specific validations like e.g. a password field where you choose a new password and it has complex password rules. In that case you can use the valid state to show the user that what they entered fullfills the rules. However the valid state should not be used for everything that is valid in a form, because this would apply to almost anything, so you would have green borders and checkmarks on all the form elements and then it's better not to specifically highlight valid state to reduce visual clutter. This will also be explained in our design document.

guoda-puidokaite commented 3 days ago

Hey, @edda. The main issue stated was about the valid and invalid states being applied at the same time. The latter was just a suggestion, thanks for the explanation. Do you think the stated issue is indeed a bug or expected?

edda commented 1 day ago

@guoda-puidokaite essentially it's more a matter of trusting people to use it correctly. We could think about changing it in such a way that if both are true, only invalid is respected. But we'll have to do this change for all form components that have the invalid and valid states then. So I'd say, add it to the collection epic for possible future improvements :D

guoda-puidokaite commented 1 day ago

Thanks, @edda. I added some possible improvement suggestions from my side here.