marigold-ui / marigold

Design System based on react-aria and Tailwind CSS
https://marigold-ui.io
MIT License
101 stars 7 forks source link

Make error prop boolean and add errorMessage in all components using error #1373

Closed ti10le closed 2 years ago

ti10le commented 2 years ago

Description

We have a few components which are using an error prop as string. We check in the components if theres no empty string and then show the error.

Context

Its not nice and safe to use the error as string. Users would also think you have to add error prop as boolean.

Consequences

Better understood props in all components. Easier and more intuitive usage of the error prop.

sebald commented 2 years ago

Isn't error just a variant?

ti10le commented 2 years ago

No not at all, we show Validation message after some input components. Like here

sebald commented 2 years ago

Bit why we are changing the current design? 🤔

ti10le commented 2 years ago

Viktoria and I are thinking that it makes no sense to have an error prop with type string. It's a result from this: https://github.com/marigold-ui/marigold/pull/1366#discussion_r723261925

ti10le commented 2 years ago

But one thing I thought about @sebald is that I've used this type in five components: export type ErrorProps = | { error?: false; errorMessage?: never } | { error: true; errorMessage?: string };

Is there a global place instead of write it in every component? I think adding in one of the components and then using from there is also not nice.

sebald commented 2 years ago

What about other states like info/warning. Do we need them too?

ti10le commented 2 years ago

I think currently we have no other component where we could do the same with warning and info. We have no other states depending on another prop.

sebald commented 2 years ago

Which one? Can the button be in the same state at the same time?

ti10le commented 2 years ago

I dont know what you want. In which component do we need this implementation, too?

sebald commented 2 years ago

Thought we are talking about the Field component.

ti10le commented 2 years ago

Oh ok, maybe we got e design for info/warning in the future? I think I know what you mean now.