Skip to content

Fix observe property string name #19411

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

Open
wants to merge 2 commits into
base: release/16.0
Choose a base branch
from

Conversation

nielslyngsoe
Copy link
Member

@nielslyngsoe nielslyngsoe commented May 24, 2025

There was a naming mistake here, which does not affect anything currently, but should be corrected — cause I think that is just a coincidence.

(This code is new for 16)

@Copilot Copilot AI review requested due to automatic review settings May 24, 2025 18:22
@nielslyngsoe nielslyngsoe requested a review from iOvergaard May 24, 2025 18:23
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

The PR corrects a reactive property name in the willUpdate lifecycle hook to ensure the component reacts to the intended change.

  • Replaced 'type' with 'property' in the changedProperties check.
  • Ensures that updates now trigger when the property input changes.
Comments suppressed due to low confidence (1)

src/Umbraco.Web.UI.Client/src/packages/content/content/workspace/views/edit/content-editor-property.element.ts:36

  • Add or update unit tests to cover the scenario where the property input changes, ensuring that the reactive update path in willUpdate is exercised.
if (changedProperties.has('property') || changedProperties.has('variantId') || changedProperties.has('_context')) {

Comment on lines 34 to +36
override willUpdate(changedProperties: Map<string, any>) {
super.willUpdate(changedProperties);
if (changedProperties.has('type') || changedProperties.has('variantId') || changedProperties.has('_context')) {
if (changedProperties.has('property') || changedProperties.has('variantId') || changedProperties.has('_context')) {
Copy link
Preview

Copilot AI May 24, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider extracting the observed property keys ('property', 'variantId', '_context') into a constant or array to avoid repeating magic strings and simplify future maintenance.

Copilot uses AI. Check for mistakes.

@nielslyngsoe nielslyngsoe enabled auto-merge (squash) May 24, 2025 18:53
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.

1 participant