Repository navigation
Feature: PV Forecast function with 3 providers - #1040
seaspotter wants to merge 4 commits into
Conversation
benderl
left a comment
There was a problem hiding this comment.
Erst einmal Danke, dass Du diese PV-Prognose implementiert hast.
Mir sind jedoch einige Details der Umsetzung nicht ganz klar und wirken auf den ersten Blick redundant oder überflüssig. Kann natürlich auch sein, dass ich hier falsch liege.
|
Ich habe mir das heute mal genauer angesehen. Es sind einige Dinge enthalten, die anscheinend durch mehrere KI-Iterationen entstanden sind. Die KI hat dann immer brav dafür gesorgt, dass eine Abwärtskompatibilität zum vorherigen Stand (z.B. eine unvollständige Konfiguration, nicht einheitliche Typenbezeichnung) sichergestellt wird. Bei diesem ersten Release für solche Forecast Module ist eine Abwärtskompatibilität jedoch nicht erforderlich, da es keinen älteren Stand gibt. Das betrifft z.B. die Methoden Dann gibt es in Architektonisch unschön ist, dass in Wenn das für Dich ok ist, nehme ich in den kommenden Tagen ein paar Anpassungen vor, um den PR mehr in Richtung "openWB Standard Code" zu bringen. |
|
Danke fürs Feedback Lutz, ich hab jetzt nochmal ein paar deiner Dinge aufgenommen und in dem Commit (505c5ab) bereinigt und nochmal einmal bei mir durchgetestet, die Funktionalität ist damit noch gegeben. Aber ja schau gerne vielleicht nochmal über diesen finalen Stand drüber, wenn noch was angepasst werden soll, dann gerne. Danke |
|
Hallo Seaspotter, Benötigt werden jetzt auch die Anpassungen aus #1113, um individuelle Symbole bei Eingabefeldern und Buttons zu ermöglichen. Nach dem Merge bitte rebasen. |
|
Danke @benderl ich hab hab das mal alles durchgetestet. Ein konkreter, reproduzierbarer Bug:
Unabhängig davon: UX-mäßig ist „wähle nichts aus der Liste" als Löschaktion nicht intuitiv – überall sonst in der App (Komponenten, Consumers etc.) gibt es dafür einen expliziten „Löschen"-Button mit Bestätigung. Genau deswegen habe ich das vorher aus UX Sicht auch mit dem "Anbieter löschen und Daten zurücksetzen" vorher so drin gehabt und würde auch dafür plädieren das beizubehalten. Zwei weiter UX-Punkte die ich in meiner Version vorher explizit so drin hatte, weil es Sinn macht und viel intiutiver:
|
743e7bf to
c83ba2e
Compare
Dann sollten wir das Caching nochmal überdenken. Ich finde es eigentlich ein gutes Feature, habe dabei aber nicht bedacht, dass es zu dieser Inkonsistenz kommen kann. Das direkte Speichern beim Wechsel des Providers ist nicht intuitiv, wenn weiter unten ein Speichern-Button zu sehen ist, daher hatte ich den Teil entfernt. Ebenfalls wäre es von der Bedienung nicht konsistent zu den restlichen Einstellungen.
Das ist nicht ganz korrekt. Bei der Prognose kann man nur ein Modul auswählen. Das ist also nicht vergleichbar mit Geräten, Komponenten oder Verbrauchern, sondern mit z.B. den Strompreisanbietern. Man kann geteilter Meinung sein, ob das so gut ist oder nicht. Mir geht es jetzt erst einmal darum, dass die Bedienung einheitlich ist. Unabhängig von diesem PR kann darüber gerne diskutiert und alternative Vorschläge gemacht werden.
Auch das Verhalten ist jetzt identisch zu anderen vergleichbaren Stellen in den Einstellungen. Aktuell kann so eine Karte beim ersten Rendern nur als auf- oder zugeklappt gesetzt werden. Ein davon abweichendes Verhalten bei bestimmten Aktionen erfordert eine Erweiterung der Kartenkomponente. Auch das kann gerne in einem folgenden PR umgesetzt werden. Stand jetzt soll es einheitlich sein.
Ja, die Info ist wichtig, da jedes Modul das potentiell anders umsetzen kann. Ich fand es nur etwas Platzverschwendung, die Info bei jeder Dachfläche immer anzuzeigen. Vielleicht einmalig in der übergeordneten Karte, in der die Dachflächen gruppoert werden? |
|
Zum Caching/„Kein Anbieter": Ich glaube wir können beides haben – Konsistenz und Caching. Vorschlag: „Kein Anbieter" bleibt im Dropdown, verhält sich aber wie jedes andere Feld (staged, erst bei Speichern wirksam) – genau wie bei Gleiches könnte man ja bei den Stromtarifen einbauen, damit es einheitlich ist. Das würde sich einfach stimmiger zu dem Entfernen alle anderen Module anfüllen und eindeutiger sein. Das löst beides: die Dropdown-Auswahl verhält sich konsistent zum Rest der Seite (dein Punkt), und die eigentliche Löschaktion bleibt explizit und sofort wirksam, statt in einer Dropdown-Auswahl versteckt zu sein. Das Caching bleibt davon komplett unberührt, da es rein über lokale State-Änderungen läuft, nicht über den Publish-Zeitpunkt. Was hältst du davon?
Aber ich meine genau das sie aufgeklappt gesetzt wird und nicht wie jetzt zugeklappt. Ja das ist global so ich weiß, aber sinnvoll finde ich es auch UX Sicht nicht: Ich drück n Plus-Button zum Hinzufügen eines Geräts und muss erstmal die Kachel aufklappen. Das ist ein Klick zuviel. Gerade wenn ich jetzt neu ein Gerät/Provider etc. hinzufüge. Wenn ich später editiere, dann ist es voll okay das es zugeklappt ist und ich direkt erstmal an das Modul hinsprungen muss.
Hatte ich auch mal überlegt, dann steht es nur wieder so allein da ohne Bezug zu nem konkreten Einstellungspunkt. Aber ja ich versteh auch das es Platzverschwendung ist. Ich fands am Ende wichtiger es immer anzuzeigen, weil es halt nicht selbsterklärend ist und ich selbst beim 120sten Mal eintippen immer noch nachsehen musste bei meinen Tests wie es jetzt richtig ist :) Aber dann würd ich vermutlich mit dir mitgehen das oben drüber global einmal anzuzeigen und als Hilfetext zugeklappt trotzdem noch bei der Einstellung zu lassen? |
📋 Überblick
Implementiert die komplette Benutzeroberfläche für PV-Prognose-Konfiguration und Überwachung,
inklusive Provider-spezifischer Formulare, Prognose-Datenvisualisierung und Statusinformationen.
🎯 Features
✅ Tests
📝 Hinweis
Enthält eine defensive Sicherheits-Check im Store (
!state.examples ||),