refactor(dashboards-ng): remove explicit editable$ subscribe in widget-host#1836
refactor(dashboards-ng): remove explicit editable$ subscribe in widget-host#1836chintankavathia wants to merge 1 commit intomainfrom
Conversation
…t-host Move setupEditable side effect into editable$ pipe via tap, leveraging the template's async pipe subscription instead of a manual subscribe in ngOnInit. Update editable$ access modifier to protected as it is meant to be used internally.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the editable state management in SiWidgetHostComponent by moving the setup logic into a pipe on the editable$ observable and removing the redundant subscription in ngOnInit. Additionally, it updates the configChange subscription to reference the grid service directly. I have provided a suggestion to improve the RxJS implementation by using withLatestFrom to make the data flow more declarative and to remove the unnecessary setTimeout.
| protected editable$ = this.gridService.editable$.pipe( | ||
| tap(editable => this.setupEditable(editable)) | ||
| ); |
There was a problem hiding this comment.
What about using fromObservable here? And then making all the things calculated in setupEditable computed or linkedSignals?
I don't like that this depends on the async pipe in the template. What happens if someone later uses this a second time in the template? Then we have duplicated execution of setupEditable. So in that regard the current approach is better. But using `fromObservable might be even nicer...
2 participants
There is already | async in template so explicit subscription is not needed here.
Move setupEditable side effect into editable$ pipe via tap, leveraging the template's async pipe subscription instead of a manual subscribe in ngOnInit. Update editable$ access modifier to protected as it is meant to be used internally.
Documentation.
Examples.
Dashboards Demo.
Playwright report.
Coverage Reports: