fix(lib): error when selectItemDirective.selected is changed by user

[banksemi]

I tried to modify the 'selected' value via the 'selectItemDirective' object. But, in this case, I found that the item(changed selecting) is not updated in container.

Issues by this problem

1. The item change event don't executed.
2. Clicking on an empty space does not clear the selection.

So I modified that if 'selected' option is changed, it call function of container.

Thank you.

[d3lm]

Hey there! Thanks for the PR but I am a little thrown off because there was no issue filed for this. I would rather have an issue first so we can discuss proposed solutions. This way we can also avoid that someone invests time in a PR that might not get merged, for whatever reason.

What exactly is the use case for this? The container already supports programatically selecting single items. Isn't that sufficient enough?

[banksemi]

Ok, I will open the issue next PR.

When I didn't understood open source, I tried change 'selected' open.

First, goal of my project was to create a checkbox like this. So I tried to connect the checkbox(input element) with a item directive in my project.

This is my screenshot.

I looked at the next part of document of open source and thought I could change the 'selected' option enough.

item.selected = !item.selected

However, this method did not work well.

Of course, I can modify the selected options by using container, but I realized later.

So I wanted to make some suggestions for users who can make the same mistake like me.

1. If the selected option is only getter value, I need clarify it in the document.
2. or, to bind 'selected' option to items of container.

If you choose option 2, Users can choose preferred method between modify the 'selected' option or using the container function.

This is the reason for my PR.

Thank you for providing a good library.

[d3lm]

Hey there! Thanks for elaborating and I now have a better picture of what you were trying to accomplish.

The selected property is indeed only meant to be a getter and not meant to be modified directly, because that would introduce a level of indirection which I was trying to avoid. This means the components had to communicate with one another and I am trying to avoid tight coupling as much as possible.

Because the container already supports selecting individual items, my suggestion, same as yours, is to update the README and emphasize that the selected property on the select-item is only a getter and when changing it has no affect on its selected state.

Would that work for you?

I mean we can also think about a proper implementation that involves a little coupling as possible. To give a little feedback on the PR you sent, you're setting the container via a setter. This means every child calls its parent when the property changed. This means each child knows about its parent and hence tightly coupling it to the parent. A slightly better approach would be to have the parent query all children, which it already does, and have the children implement some sort of selection stream that the parent can subscribe to. Maybe this is slightly better? Curious to hear your opinion on this.

@timdeschryver Any input from your side?

Thank you for providing a good library.

I am happy to hear you like the library and find it useful!

[timdeschryver]

I'm in agreement here with @d3lm, indirection can lead to one big mess.

For now, my vote goes to updating the README. If needed, we can still revisit it later and implement the suggestion you suggested @d3lm .

[banksemi]

Thank you for the good feedback.

Actually, I worried about the dependency circulation while doing PR. But, if accept @ d3lm's suggestion, it may be able to avoid a level of indirection and dependency circulation.

I want to implement this.

Can you review the revised PR tomorrow?

[banksemi]

I updated PR. I would like to hear your opinion about the changed PR. @timdeschryver @d3lm Thank you.

[d3lm]

Hey there! @banksemi Do you mind reverting your PR and only change the README for now mentioning that the selected property of the select item is only readonly? For now I would like to avoid that you can directly change the state of an item on the item itself.

[d3lm]

Because on inactivity I am closing this PR. In case you wanna change your PR to add a small note to the README, we can again reopen this one.