-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Ensure min/max property editor settings are valid before rendering for content editing #17881
base: v15/dev
Are you sure you want to change the base?
Ensure min/max property editor settings are valid before rendering for content editing #17881
Conversation
…r content editing.
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.
Great work, @AndyButland. I am wondering if this shouldn't be a task for the input component to verify rather than the property editor? My thinking is that you could be using the input element directly elsewhere and you wouldn't get any validation of the inputs.
If we look at the <umb-input-multi-url />
as an example, it sets the min and max up through a range validator:
this.addValidator(
'rangeUnderflow',
() => this.minMessage,
() => !!this.min && this.urls.length < this.min,
);
this.addValidator(
'rangeOverflow',
() => this.maxMessage,
() => !!this.max && this.urls.length > this.max,
);
In my mind, it would be perfectly reasonable to "activate" the validator if the conditions are invalid, i.e. if min > max
then there is a rangeOverflow thus displaying the validation error and refuse to submit.
An alternative could be to implement an _errorMsg
as a state and display that with a red text color - this could also be done in the input element rather than the property editor.
What are your thoughts towards this?
Does sound like it would be better. I copied this pattern from an existing property editor that did similar - |
Great to see some activity on this matter, @AndyButland Adjacent to what @iOvergaard writes about, then notice for a Property Editor UI to consider an inner element, like an Input. it needs to be bind. It can be done in this way, as code of the Property Editor:
So you have to actively bind Elements to the validation system. In this case the Property Editor UI is bound already, but the implementation needs to define wether the inner element should be taking into account. Doing so will also make the inner validation message bobble up to the Property Editor and thereby display the message in a red text. So Ideally it should never be necessary to display validation texts on your own. — I can also hear this would be good to document, but also please respect that such would take some time, so I will postpone that further, there is a document on the very technical side of things here, but that is also very much only related to if you want to implement the system n your own, so not relevant for Property Editors. Thanks for your attention |
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.
disapproving again (was testing something regarding github)
Prerequisites
Fixes #17509, tracked under AB 47508 (internal HQ tracker).
Description
The issue raises the point that it's possible to save invalid combinations of min and max values when configuring data types with various property editors.
This PR isn't a full fix for the issue. With the changes in place, the invalid values will still be saved. But with this PR merged we'll convert invalid pairs of values to valid ones before rendering them for content editing, and throw an error visible in the browser console indicating the problem.
This is the same pattern followed on a couple of other property editors, such as the slider.
A better fix would be to have some validation available on saving data type configuration, where we could provide custom code for each to validate across the configuration values. But it doesn't look like we have a good solution for that yet - unless I've missed it, but couldn't see use cases - and so this would likely be something to tackle as a feature.
To Test: