appsmithorg / appsmith

Platform to build admin panels, internal tools, and dashboards. Integrates with 25+ databases and any API.
https://www.appsmith.com
Apache License 2.0
33.93k stars 3.66k forks source link

[Bug]-[2752]: Missing validation for colors #6687

Open ramsaptami opened 3 years ago

ramsaptami commented 3 years ago

Description

When user inputs an invalid hex code, e.g. #789945### then colour does not change since it's an invalid code (expected behaviour) but it does not prompt for an error nor revert back to a valid hex code format (expected behaviour that needs to be added)

Important Details

LOOM DEMO

ramsaptami commented 2 years ago

Hey @AsianCat54x, this isn't a great first issue. Can you look at others that has the tag "Good First Issue"?

vuiets commented 2 years ago

Stats

Stat Values
Reach 344
Effort (months) 0.25

For reach, People using the color property on a widget in the last 6 months: 1719 Expect about 20% to key in the wrong hex code value: 344

rahulramesha commented 2 years ago

The Color picker accepts not only hexValues but also any text that browser accepts as a Color like "transparent", "blue" or even "rgba(230,220,210,10)". If we restrict now to make it only accept a certain specific values or types, we might miss a scenario where in a previously working app might break. @rohitagarwal88 Not sure how to move forward with this, thoughts?

rohitagarwal88 commented 2 years ago

@rahulramesha : Check the google chrome colour picker. We can follow the same approach here. https://www.loom.com/share/a362767ba80043dbb0dbfa3719a92c06

rahulramesha commented 2 years ago

@rohitagarwal88 Throwing an error if it is the text does not resolve to a color might break people's apps, Something like the one you showed can work, like "not updating the color if it is an invalid color"

vuiets commented 2 years ago

Thinking out aloud. Would showing an error message for invalid color inputs in the debugger make sense here?

rahulramesha commented 2 years ago

@vuiets yes it does, But with the past experience on propertyPane controls, whenever a property was left unrestricted and then we decided to add restrictions it usually breaks user's apps. Have seen this happen too many times, wherein we had to roll back the restrictions. @riodeuno what do you think?

vuiets commented 2 years ago

Agree with your observation here @rahulramesha. We don't have to do anything that is blocking or breaking the app here. The main problem here is nothing happens and one doesn't know what is wrong. So in case of a wrong input, we should atleast inform people that their entry is invalid.

Like you suggest we could not update the color when it is invalid but atleast warn them that it is invalid in the debugger so they are aware and informed of the issue? Any downsides to doing that?

rohitagarwal88 commented 2 years ago

d2400b67-fba8-4322-b20e-621f62ec97a9.webm @vuiets / @rahulramesha : I agree with @vuiets opinions. Check the retool error handling for color picker. That is inline with @vuiets expectation