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

v15: Remove old values from checkboxlist #17936

Open
wants to merge 2 commits into
base: v15/dev
Choose a base branch
from

Conversation

Zeegaan
Copy link
Member

@Zeegaan Zeegaan commented Jan 10, 2025

Fixes #9417

Notes

  • This removes values that are not in the prevalues in the checkbox list

How to test

  • Follow steps in original issue.

@Zeegaan Zeegaan requested a review from iOvergaard January 10, 2025 12:00
this.#selection = this.#selection.filter((selectedItem) =>
this._list.some((listItem) => listItem.value === selectedItem),
);
this.dispatchEvent(new UmbPropertyValueChangeEvent());
Copy link
Member

Choose a reason for hiding this comment

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

Hi @Zeegaan I would recommend making sure we only fire this change event if the configuration did make a change. I think you could compare length before and after?

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.

I am wondering whether this is the best solution to the editor person updating the values. What happens with this change is that any persisted values, that are not found in the configuration are removed, but only if you go to all content pages and hit Save. We cannot do anything about the content in this case, i.e. we cannot and will not make a content migration. However, just removing the old "invisible" values also doesn't seem to tell the user what is going on, e.g. when the user hits Save, we are just removing the old values behind-the-scenes.

What if we instead showed the user the old values in a way that would indicate, that they are indeed still stored, but the user has to make the active choice to remove them? This could probably be designed in a way to make a clear indication that old values would trigger the validation system to mark the field as invalid?

@nielslyngsoe any thoughts on this?

@Zeegaan
Copy link
Member Author

Zeegaan commented Jan 13, 2025

@iOvergaard I'm not opposed to that idea, but that is out of scope for my front-end competencies 🤣
I've made a backend validator now as well, so you will also get a validation error when you have invalid values 😁💪
image

@iOvergaard
Copy link
Contributor

@iOvergaard I'm not opposed to that idea, but that is out of scope for my front-end competencies 🤣 I've made a backend validator now as well, so you will also get a validation error when you have invalid values 😁💪

Now with your backend validator and automatic mapping to the frontend validation system, we might not need frontend validation for the part, but let's see - good idea!

Let's see what @nielslyngsoe thinks about showing the invalid values so that you can still save your content without losing any data before you have implemented an alternative.

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.

4 participants