-
Notifications
You must be signed in to change notification settings - Fork 2.5k
TextField color editor should accept default value with #RRGGBB format #18122
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
src/OrchardCore.Modules/OrchardCore.ContentFields/Settings/TextFieldSettingsDriver.cs
Show resolved
Hide resolved
efc6951
to
5187039
Compare
@hishamco This will add a model state error when someone tries to set it to Here: OrchardCore/src/OrchardCore.Modules/OrchardCore.ContentFields/Views/TextField-Color.Edit.cshtml Line 10 in e8d239b
|
Yes, because that's the mean reason when you set any value with such format it shows black all the time
I will double check, I am afraid that I did it in other PR ;) |
hmm, I maybe made a mistake because the default value is maybe affected in the driver. I will pull your PR and test it. |
Actually, the string format should be #XXX to a minimum and #XXXXXXXX to a maximum.
|
So, I'm not sure if the modelstate error makes sense because it should validate only when the value is required. Else an empty string should be allowed. But here is my point, this color editor doesn't allow to return a string empty which is why I was looking to "negate" that color with a color picker that allows to set also opacity. That's kind of a hack but that would have worked. Else, we need a checkbox to display the color picker to pick a color or not. if field is required = show checkbox before color picker if we want opacity picker = maybe use a javascript component that allows it. Or set a third textbox to enter the value as text. |
I agree but due the limitation of the input type color, we can't do that unless we do the conversion behind the scene https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Elements/input/color |
Yeah, I think we need a javascript component because using a separate component for the opacity will look bad. Not to mention that it needs to support single char opacity for a 4 chars color hex and also 2 chars for the 8 char hex. I think the input type="color" is simply not enough. And the reason why the input type="color" return |
I could tweak this PR to support both |
@Skrypt I added the support for |
// Convert colors with #RGB format to #RRGGBB, because the color editor will use black color all the time. | ||
if (contentPartFieldSettings.Editor == TextFieldColorEditor) | ||
{ | ||
if (!HexColorRegex().IsMatch(model.DefaultValue)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally we should do a check if (model.Required == true)
too here. But only if we add a checkbox on the view to allow returning a null value "or empty string".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allow returning null
might break the input color
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, if we have opacity it is fine enough for now. We can take a look at adding an option to the "coloris" js component later on. Need to see if community would agree too.
At the same time, these HTML input types are really ugly and too simple. I would rather use that "coloris" component right away, but that's me. 😉
I did a quick research and I think this component would work: https://github.com/melloware/coloris-npm Different pull request though. I tested this color picker and it allows to set it to an empty string value. |
It seems that this pull request didn't really move for quite a while. Is this something you'd like to revisit any time soon or should we close? Please comment if you'd like to pick it up. |
This pull request has merge conflicts. Please resolve those before requesting a review. |
If we also want this validation of the default value in the text field driver, it must be slightly updated because of PR #18361. The default value for the color picker now supports all formats, including the alpha channel:
Also, a default value is required for the input tag, but the color editor takes care of that. So in the settings, a default value is not required anymore. |
@gvkries because I didn't follow up with you related PR, is there a need for this PR or should I react to your change then we could merge this? |
It's not required, but I think this is still nice to have. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not necessary to convert the default value to the format #RRGGBB
, this is now handled by the editor. In addition, the default value should allow all possible formats, including those with an alpha channel.
Fixes #18120
Now we are not accepting color in #XXX format to prevent displaying black color all the time