-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Fix the default value condition in NumericFieldDisplayDriver #12972
Fix the default value condition in NumericFieldDisplayDriver #12972
Conversation
src/OrchardCore.Modules/OrchardCore.ContentFields/Drivers/NumericFieldDisplayDriver.cs
Show resolved
Hide resolved
Please check the other settings/fields/parts. |
We already had discussions on default values so not sure I agree with this PR. Normally when we import items we populate data and use handlers for validations, not drivers. But if you want to provide an editor experience maybe a wrong usage of build and update editor methods where isNew = true means that the Default value should be used, these methods are intended to be used for creating new items or editing existing ones. To fit your use case where parts/fields are pre-populated, I think you need a custom impletention where you may have to build an editor with isNew = false. As a side note I saw your demo about Excel import, what you can use for a long running process is the |
Yes I remember a little a discussion around NULL which can be considered as a value, for example when we edit and then submit an empty string that is mvc bound to a null value. Then when we edit again we may see that So not sure it's good to change the current pattern, I'm quite sure that if we fix the use case related to this PR we will break other use cases. |
@jtkech this should not break anything. Because if you are using what we have today, then passing DefaultValue should be applied ONLY if there is no preset value. We should respect existing value. |
How do we insert LMAO gifs here? |
Okay, maybe because I'm used to see you fully sure too quickly, but here it seems that you're right, so just to say to be careful ;) |
@sebastienros "this should not break anything" != "this will not break anything".... Glad to see you interacting out side of the scope of meetings... From what I can see here is the only other field settings that uses default value is The |
@jtkech I think default values in this case are fine because there is a check that the property is null or has no value. If it had a value it would not use the default one on imports. The only caveat is that we assume that the way to import a field's property with an empty value is to use "" for instance and not null. |
@sebastienros Okay then |
Fix #12971