Skip to content
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

Merged

Conversation

AndyButland
Copy link
Contributor

@AndyButland AndyButland commented Jan 2, 2025

Prerequisites

  • I have added steps to test this contribution in the description below

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:

  • For the numeric, multi URL and text area property editors, create data types with invalid combinations of min/max values (i.e. min > max).
  • When using the data types in content editing, verify that a working property editor is rendered and a console error is thrown.

Sorry, something went wrong.

…r content editing.
Copy link
Contributor

@iOvergaard iOvergaard left a 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?

@AndyButland
Copy link
Contributor Author

Does sound like it would be better. I copied this pattern from an existing property editor that did similar - property-editor-ui-slider.element.ts - but perhaps there too we could fix in a similar way rather than how it's currently done. I'll have a look to see if I can make it work along the lines you suggest.

@nielslyngsoe
Copy link
Member

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:

	override firstUpdated() {
		this.addFormControlElement(this.shadowRoot!.querySelector('umb-input-number-range')!);
	}

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.
Well, it depends, cause an editor like the Block Grid Editor does have inner Validation System implemented, but for the simpler Editors it should not be needed.

— 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.
src/packages/core/validation/README.md

Thanks for your attention

@nielslyngsoe nielslyngsoe marked this pull request as draft January 8, 2025 14:38
@nielslyngsoe nielslyngsoe marked this pull request as ready for review January 8, 2025 14:38
@nielslyngsoe nielslyngsoe self-requested a review January 8, 2025 14:39
Copy link
Member

@nielslyngsoe nielslyngsoe left a 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)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…or-config
@AndyButland
Copy link
Contributor Author

I've had a further stab at this @iOvergaard, but not really sure about it.

I couldn't get the this.addValidator approach to work. It doesn't seem that the existing validation on the multi-URL picker works either actually.

So I went for the approach of displaying a message if the configuration is invalid. That works for the multi-URL picker, but the styling I've just lifted from somewhere else a similar block is displayed in the code base, so likely that needs a tweak or at least being it's own component.

But then moving onto the other two editors - for the number and text areas - I couldn't apply these to the inputs as they are defined in the uui library. So not sure if it's best to update them there? Or go back to doing the updates in the property editors themselves.

@leekelleher leekelleher self-assigned this Jan 14, 2025
@leekelleher leekelleher self-requested a review January 14, 2025 08:22
@iOvergaard
Copy link
Contributor

@leekelleher, I noticed you self-assigned this. Will you look at @AndyButland's comment and try to figure it out?

@leekelleher
Copy link
Member

After reviewing the PR and chatting with Niels about it, we noted that with the cause of the issue being a misconfiguration of a Data Type, that displaying an error on the Property Editor would not be the correct approach.
An on-screen error/alert may seem unfriendly for a content-editor user; who may be unaware how an implementor/developer has configured the CMS.

As noted above, it would be a better fix to have validation across multiple fields in the Data Type configuration, however we don't have a mechanism for that yet.

For now, I've amended the code in this PR to put a console warning for the min/max misconfiguration. I opted with console.warn instead of an Error so not to disrupt code execution.

Unfortunately, this does not fix issue #17509. We'll need to work towards a solution that involves Data Type validation or combining the min/max fields to use a number-range input (as used on the Block editor configurations).

@leekelleher leekelleher requested review from iOvergaard and nielslyngsoe and removed request for leekelleher January 20, 2025 16:35
…or-config

# Conflicts:
#	src/Umbraco.Web.UI.Client/src/packages/property-editors/number/property-editor-ui-number.element.ts
@iOvergaard iOvergaard merged commit 85a81b3 into v15/dev Jan 31, 2025
29 checks passed
@iOvergaard iOvergaard deleted the v15/bugfix/validate-min-max-property-editor-config branch January 31, 2025 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants