Skip to content

Slider: fix duplicated property editor settings properties#22898

Merged
nielslyngsoe merged 4 commits into
release/17.4.1from
v17/hotfix/slider-remove-duplicate-range-minimum-property
May 19, 2026
Merged

Slider: fix duplicated property editor settings properties#22898
nielslyngsoe merged 4 commits into
release/17.4.1from
v17/hotfix/slider-remove-duplicate-range-minimum-property

Conversation

@nielslyngsoe

Copy link
Copy Markdown
Member

Removes duplicate Property Editor settings and adds a client-side error when it happens.

Copilot AI review requested due to automatic review settings May 19, 2026 13:13
@claude

claude Bot commented May 19, 2026

Copy link
Copy Markdown

Claude finished @nielslyngsoe's task in 1m 59s —— View job


PR Review

Target: origin/release/17.4.1 · Based on commit: be30228e

Removes the duplicate minimumRange property setting from the Slider UI manifest (it was already defined in the schema manifest), adds a defensive duplicate-alias guard in the data type workspace context, and improves the minimumRange description.

  • Other changes: Slider "Minimum range" PE setting description is now populated (was empty string); the duplicate check in #mergeConfigProperties() is a general guard that will fire for any property editor with duplicate aliases, not just slider.

Suggestions

  • data-type-workspace.context.ts:263: throw new Error(...) is called inside an RxJS subscriber callback (via observe()). If source.subscribe(callback) propagates an uncaught synchronous throw, it may crash the workspace rather than showing a developer-friendly error. For this type of defensive/configuration guard, console.error() paired with a user-facing notification would be more resilient — the workspace could still render while surfacing the misconfiguration. Since the slider bug itself is fixed, this only matters if another property editor introduces the same mistake in future.

    // More resilient alternative:
    if (aliasSet.has(setting.alias)) {
        console.error(`Duplicate alias "${setting.alias}" in Property Editor configuration.`);
        // optionally: UmbNotificationContext notification
        continue; // or break, depending on preferred behaviour
    }

Approved with Suggestions for improvement

Good to go, but please carefully consider the importance of the suggestions.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes duplicate minimumRange settings property on the Slider property editor (declared both in the schema manifest and the UI manifest) and adds a defensive client-side check in the data type workspace that throws when duplicate setting aliases are merged from schema/UI/data-source manifests.

Changes:

  • Remove duplicated minimumRange property and its default value from the Slider UI manifest, keeping the canonical definition on the schema manifest (with the descriptive text moved to the schema).
  • Add duplicate-alias detection in UmbDataTypeWorkspaceContext.#mergeConfigProperties that throws when two merged settings share an alias.
  • Minor refactor in #transferConfigDefaultData: rename the loop variable from defaultDataItem to property and remove an outdated TODO comment.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
src/Umbraco.Web.UI.Client/src/packages/property-editors/slider/Umbraco.Slider.ts Adds a meaningful description to the schema's minimumRange property.
src/Umbraco.Web.UI.Client/src/packages/property-editors/slider/manifests.ts Removes the duplicate minimumRange property and default-data entry from the UI manifest.
src/Umbraco.Web.UI.Client/src/packages/data-type/workspace/data-type-workspace.context.ts Adds duplicate-alias detection on merged settings and renames a loop variable for clarity.

@claude claude Bot added area/frontend category/ux User experience category/ui User interface labels May 19, 2026
@nielslyngsoe nielslyngsoe changed the title fix duplicate slider pe-settings properties Slider: fix duplicated property editor settings properties May 19, 2026
@nielslyngsoe nielslyngsoe enabled auto-merge (squash) May 19, 2026 13:24
@nielslyngsoe nielslyngsoe merged commit ba29b91 into release/17.4.1 May 19, 2026
26 of 27 checks passed
@nielslyngsoe nielslyngsoe deleted the v17/hotfix/slider-remove-duplicate-range-minimum-property branch May 19, 2026 14:19
AndyButland pushed a commit that referenced this pull request May 19, 2026
* fix duplicate slider pe-settings properties

* remove comment

* avoid throws
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.

3 participants