Feat: Graph modal upgrade - flex-config integration#1926
Feat: Graph modal upgrade - flex-config integration#1926joshuaunity wants to merge 19 commits intofeat/allow-Ssensorstoshow-schemafrom
Conversation
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Documentation build overview
Show files changed (15 files in total): 📝 14 modified | ➕ 0 added | ➖ 1 deleted
|
… side, including dissabling logic Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
…sures/flexmeasures into feat/sensortoshow-modal-upgrade
…sures/flexmeasures into feat/sensortoshow-modal-upgrade
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
…eatures Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
…sures/flexmeasures into feat/sensortoshow-modal-upgrade
…sures/flexmeasures into feat/sensortoshow-modal-upgrade
nhoening
left a comment
There was a problem hiding this comment.
Good start!
I noticed these things when I tested:
Also, the "Asset Config" tab should be the first option: on the left, open per default. (I believe we want users to use those fields if possible, and use the sensor search if they need more than that). And I would call it "Flex Config".
When selecting an asset, I sometimes see "flex-model" twice under Config-Type.
When I select a different config-type, previously selected field & details should be emptied. Actually, it seems the list of available fields is not replaced, as well.
Some help texts.
- Next to "field" let's have an info icon, explaining that here, fields can be selected (Q: do you only show fields withUnder "Field Details", let's
I wanted to remove such a new field, but nothing happened, also no error.
Removing a traditional field got me "Uncaught ReferenceError: removeSensorFromGraph is not defined"
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
…n. Also added some docstring Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
nhoening
left a comment
There was a problem hiding this comment.
Additions I'd like to see:
- Can we add the flex-config information to the plot legends? Now I see "sensor-name (asset-name)" in the legend. But if I selected a sensor for its function in the flex-model, we should make that information visible, as to make the UX clearer. So in the case I selected soc-min, I'd like to see "sensor-name (soc-min for asset-name)". I know this might be a bit tricky to accomplish. But it's important for users to see why a plot was added.
- On compontens.js, I asked for docstrings of the functions, but I believe you only added one.
- The form for flex config settings should have some info icons for help. E.g. which assets are shown in the dropdown? Will fields without value be hidden?
Some things that don't seem to work properly:
- I am actually confused which assets show up in the list - as I am testing it is a short list. I believe it is short because it is limited by pagination (
&per_page=10). So that limitation is wrong. But also the set of assets being offered - the issue says: "assets are all assets in the same tree as the currently edited one". For this, the newrootparameter of that asset endpoint can be used. On that issue, you discussed with us how to do this. - Searching for sensors (old-fashioned) gives JS error: "Uncaught ReferenceError: filterSensors is not defined"
- Also, when I select a graph on the left, the button to add a sensor used to switch its text, but now it keeps saying "Add new Graph"
i caught this yesterday as well, i havent pushed my updates yet
This is in progress
Yes, you are correct, to speed up development on the initial phase, i just fetched the sensors. I will implement this feature now as i did forget
I thought you were referring to only the util function, i can add for the rest too. |
… revive other broken features Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
|
@joshuaunity let me know when this is ready for review |
You can go ahead and review, as I'm currently working on the fixed value graphs |
nhoening
left a comment
There was a problem hiding this comment.
Actually, at least half of my previous review has not been addressed. So then you should not tell me it's okay to review.
Please address these open items from the last review:
- Can we add the flex-config information to the plot legends? Now I see "sensor-name (asset-name)" in the legend. But if I selected a sensor for its function in the flex-model, we should make that information visible, as to make the UX clearer. So in the case I selected soc-min, I'd like to see "sensor-name (soc-min for asset-name)". I know this might be a bit tricky to accomplish. But it's important for users to see why a plot was added.
- The form for flex config settings should have some info icons for help. E.g. which assets are shown in the dropdown? Will fields without value be hidden?
- Searching for sensors (old-fashioned) gives JS error: "Uncaught ReferenceError: filterSensors is not defined"
Also, I found some other things which should be improved:
- There are fields which are booleans (e.g.
' prefer-charging-sooner). Let's not offer those. - I wanted to remove a field from a graph (clicking "x"), but that did not work. No error in the console.
- There is logs in the console which should go away:
- "Rendering graph cards..."
- "Rendering API sensors..."
| async function renderRootAssets() { | ||
| const assetTreeSelect = document.getElementById("assetTree"); | ||
| const response = await fetch(`${apiBasePath}/api/v3_0/assets?page=1&per_page=10&sort_by=id&sort_dir=asc`); | ||
| const rootAssetId = "{{asset.id}}" |
There was a problem hiding this comment.
The root asset should be the one highest up in the asset tree, not the one that is being worked on. In my last review I pointed you to the issue clearly saying that and also to code which you can use to find it, which @Flix6x had looked up.
Read what the review says.
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
|
The following are left
|
Okay, if you get them done by tomorrow I can probably still review. And you said fixed values (e.g. "30 kW") are already worked on in this PR? |
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Yes, I just made the second phase of major changes, i think this would also be the last phase, so you can check out the feature. |
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Signed-off-by: joshuaunity <oghenerobojosh01@gmail.com>
Description
This PR adapts the new
SensorsToShowSchema, which introduced the concept of plots and aset flex-config references.Look & Feel
Before

After

How to test
Further Improvements
Related Items
This PR Closes #1881
Sign-off