Implemented CSS style bindable #572
Conversation
| try | ||
| { | ||
| var value = GetValue(styleProperty).ToString(); | ||
| writer.AddStyleAttribute(styleProperty.GroupMemberName, value); |
There was a problem hiding this comment.
When the value is null or empty, the style property is not set by knockout. The null case is actually consistent with behavior of this, but I think that we could do that without the NullReferenceException ;)
Also note that when the value contains invalid stuff (like a malitious input), the assignment is ignored on client side but can do some harm on the server. I'm not sure if we can do anything about it, but it's worth noting.
| @@ -51,6 +51,11 @@ public HtmlGenericControl(string tagName, bool allowImplicitLifecycleRequirement | |||
| public static DotvvmPropertyGroup CssClassesGroupDescriptor = | |||
| DotvvmPropertyGroup.Register<bool, HtmlGenericControl>("Class-", "CssClasses"); | |||
|
|
|||
| public VirtualPropertyGroupDictionary<object> StyleProperties => new VirtualPropertyGroupDictionary<object>(this, StylePropertiesGroupDescriptor); | |||
There was a problem hiding this comment.
I would rename it to CssStyles - just to make it similar to CssClasses.
There was a problem hiding this comment.
ok, let's be consistent...
| var value = GetValue(styleProperty)?.ToString(); | ||
| if (!string.IsNullOrEmpty(value)) | ||
| { | ||
| writer.AddStyleAttribute(styleProperty.GroupMemberName, value.ToString()); |
There was a problem hiding this comment.
You don't need to call ToString here.
There was a problem hiding this comment.
You're right, I forgot that after the last changes
| foreach (var styleProperty in CssStyles.Properties) | ||
| { | ||
| if (cssStylesBindingGroup == null) cssStylesBindingGroup = new KnockoutBindingGroup(); | ||
| cssStylesBindingGroup.Add(styleProperty.GroupMemberName, this, styleProperty); |
There was a problem hiding this comment.
This will add hard-coded values to binding group. I think it's not necessary because hard-coded values are added to style attribute and can't be changed.
There was a problem hiding this comment.
Good point. I changed it accordingly. thank you
There was a problem hiding this comment.
Ok, but it should be good enough to set the nullBindingAction. You don't need to call HasValueBinding.
There was a problem hiding this comment.
Or do you want to avoid rendering of empty binding group?
There was a problem hiding this comment.
I see. But yes, I wanted to avoid rendering style data-bind when there are only hard-coded values. I didn't know about nullBindingAction and it's somehow strange that you have to allocate delegate to do nothing.
There was a problem hiding this comment.
Ok. Can you also change CssClasses to not render empty group in this case?
adamjez
force-pushed
the
features/css-style-bindable
branch
from
810c564
to
cfccffc
Compare
adamjez
force-pushed
the
features/css-style-bindable
branch
from
cfccffc
to
f1c7672
Compare
|
thanks for review, I'm merging it. |
Added
Style-*property group that generates thestyleknockout binding. I'm not sure what type should I use for the property group, currently there isobject.Usage example:
Resolves #569