Config UI: filter parameters by usage #21821
Conversation
There was a problem hiding this comment.
Hey @Maschga - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `assets/js/components/Config/MeterModal.vue:274` </location>
<code_context>
return params.filter((p) => !CUSTOM_FIELDS.includes(p.Name));
},
normalParams() {
- return this.templateParams.filter((p) => !p.Advanced);
+ return this.templateParams.filter(
</code_context>
<issue_to_address>
Duplicate filtering logic across modal components
Consider extracting the shared filtering logic into a utility or mixin to reduce duplication and enhance maintainability.
Suggested implementation:
```
normalParams() {
return filterTemplateParams(this.templateParams, this.templateType, false);
},
advancedParams() {
return filterTemplateParams(this.templateParams, this.templateType, true);
```
```
<script>
import { filterTemplateParams } from '@/utils/templateParamFilters';
export default {
computed: {
```
You need to create a new file at `assets/js/utils/templateParamFilters.js` (or `.ts` if using TypeScript) with the following content:
```js
export function filterTemplateParams(params, templateType, advanced) {
return params.filter(
(p) =>
!!p.Advanced === !!advanced &&
!p.Deprecated &&
(p.Usages && templateType ? p.Usages.includes(templateType) : true)
);
}
```
Make sure to adjust the import path if your project structure differs.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@Maschga magst du einen playwirght test ergänzen der das prüft. als einmal ein pv-meter und ein battery-meter erstellen und prüfen dass es die richtigen Optionen gibt. dafür können wir ruhig ein "echtes" template verwenden. brauchen ja nicht speichern/anlegen. |
|
Dürfen die beiden Tests auch hier rein? evcc/tests/config-battery.spec.js Lines 19 to 95 in d1d2375 Lines 17 to 70 in d1d2375 |
Ja, braucht kein eigener Testcase sein. In einen bestehenden integrieren ist immer besser. Ggf. nen Kommentar hinzufügen wenn unklar ist wieso der Extracheck im Test ist. |
|
Sind die Tests so ausreichend? |
| description: | ||
| en: Maximum charge power | ||
| de: Maximale Ladeleistung | ||
| advanced: true |
There was a problem hiding this comment.
alternativ kannst du dem soc (drüber) einfach eine usage verpassen
There was a problem hiding this comment.
In demo-meter.yaml sehe ich keinen soc. 🤔 Meinst du energy oder power?
There was a problem hiding this comment.
Wenn wir Max Ladeleistung haben dann wäre auch Soc auf jeden Fall sinnvoll ;)
There was a problem hiding this comment.
Auf welche Frage bezieht sich dein Ja?
There was a problem hiding this comment.
Aber fürs Testen reicht maxchargepower aus, oder nicht?
Ja.
oder darum, maxchargepower durch einen bereits vorhandenen Parameter wie energy zu ersetzen und bei energy usages: ["battery"] hinzuzufügen?
Nein weil das physikalisch unsinnig wäre, demo hin oder her.
There was a problem hiding this comment.
Deshalb hatte ich soc vorgeschlagen- den braucht es in der realen Welt immer und er passt gut zu einer demo battery.
There was a problem hiding this comment.
maxchargepower wurde jetzt durch minsoc ersetzt.
Co-authored-by: Michael Geers <michael@geers.tv>
Co-authored-by: Michael Geers <michael@geers.tv>
Fix #21814