Conversation
mhdirkse
commented
Mar 10, 2026
mhdirkse
commented
- Replace test tab settings component by single button because the only option is to show/hide storage ids.
- Respect choice in debug tab for report generator enabled - remove additional logic that obscures this.
- Do not omit reports with empty XML when downloading from report component or when downloading from test tab. These download buttons do not show the checkbox.
Matthbo
left a comment
Matthbo
left a comment
There was a problem hiding this comment.
I dont see why the booleanToString pipe exists when it's literally a ternary which you can do in the angular template.
You even use a ternary in this PR for the same thing.
| </div> | ||
|
|
||
| <p>Generator is {{ generatorEnabled|booleanToString: 'enabled':'disabled' }}</p> | ||
| <p>Generator is {{ this.settingsService.isGeneratorEnabled()|booleanToString: 'enabled':'disabled' }}</p> |
There was a problem hiding this comment.
| <p>Generator is {{ this.settingsService.isGeneratorEnabled()|booleanToString: 'enabled':'disabled' }}</p> | |
| <p>Generator is {{ this.settingsService.isGeneratorEnabled() ? 'enabled':'disabled' }}</p> |
There was a problem hiding this comment.
Deze booleanToString pipe is onderdeel van de Ladybug code, niet een import uit een library. Hij wordt intensief gebruikt. Ik haal hem hier weg. Ik ga hem op termijn wegwerken maar liever niet als onderdeel van deze PR.
There was a problem hiding this comment.
Oke, Ik snap niet hoe de pipe ooit uit een PR review is gekomen maar het is niet bepaald meer de standaard die we zouden verwachten.
Overigens is de pipe vervangen simpel weg een tenary statement gebruiken op alle plekken dus ik verwacht wel dat het met een refactor of cleanup van de code dit opgepakt gaat worden.
2 participants
