ref: separate ui and business logic #3
Conversation
| private calcSelectedLNClasses(): string[] { | ||
| const somethingSelected = this.selectedLNClasses.length > 0; | ||
| const lnClasses = this.lnClassList.map( lnClassInfo => lnClassInfo[0] ); | ||
|
|
||
| let selectedLNClasses = lnClasses; | ||
|
|
||
| if(somethingSelected){ | ||
| selectedLNClasses = lnClasses.filter( lnClass => this.selectedLNClasses.includes(lnClass)); | ||
| } | ||
|
|
||
| return selectedLNClasses; | ||
| } | ||
|
|
There was a problem hiding this comment.
This is the main logic.
I've went for readability. You could argue, that we could wait with the mapping until nothing is selected, but I don't think this so such a big performance problem and it would be the code less readable.
|
|
||
| this.selectedLNClasses = selectedItems; | ||
| this.selectedLNClasses = e.detail.selectedItems; |
| @@ -214,8 +216,7 @@ export default class IedPlugin extends LitElement { | |||
| const label = lnClassInfo[1]; | |||
| return html`<mwc-check-list-item | |||
| value="${value}" | |||
| ?selected="${this.lNClassListOpenedOnce && | |||
| this.selectedLNClasses.includes(value)}" | |||
| ?selected="${this.selectedLNClasses.includes(value)}" | |||
There was a problem hiding this comment.
Less sure about reverting the "opened once" logic.
There was a problem hiding this comment.
I really like the improvement and I checked this out and had a look.
However when I select a new IED then all LNs are selected again.
- Open file.
- Open LN class filter. Looks good.
- Close LN filter.
- Open IED filter.
- Select new IED
- Open LN filter.
- Alas, all LNs are selected
Also, there is another issue alas which is quite visible when I merge this PR which is that
- Select some LNs in the LN class filter.
- Change the IED using the IED filter.
- Open the LN class filter -- the same LNs are selected.
- However the selection is not correctly being applied.
There was a problem hiding this comment.
How about like this? (PR has been updated)
|
Thanks @trusz for looking at this for me -- would you like to make further improvements or I can try to adapt for the two scenarios which still appear to give unexpected results. Sorry, this seems quite fiddly and not well wrapped in test coverage either. |
|
Thanks @trusz it is better. and a better way of structuring this. Alas it is still not right for the behaviour you were looking for... I can get a situation where I select all all LNs and I am expecting to see none selected when I open the LN filter which was (I thought) the most difficult problem.... still_not_right.mp4I had another play to try and resolve this but was not successful. |
|
Small ping @trusz ! 😉 |
|
It looks good to me. This would have been my expectation. The UI stays es the user left it. It should not change its state because of a single case. Two states has the same effect, which you would not want normally but I think this is an exception.
|
|
Hmmm, I thought you wanted if the user selects everything for there to be nothing selected based on your previous comment but I am happy with it as it is, so let's leave further improvements for another time. |
No description provided.