-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat: Asset context overview page. #1369
base: main
Are you sure you want to change the base?
Conversation
treeSpecs, | ||
{renderer: "svg"} | ||
); | ||
function openModal(data) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! At this time, I still see this as a PoC, but it's promising. My comments deal mostly with the tree.
On my side, I have to figure out how to make this page look okay if there is just one asset. You seem to be experimenting with the simulation structure, which is naturally rich, but in the EMS, people might often (or at first) just have one or two assets.
First, there are some display issues with the tree:
For me, on Firefox, there is horizontal jiggling when I hover the tree. On both Firefox and Chrome, the initial size is smaller than it is after I hovered for the first time.
Here is a video made on Firefox:
vokoscreenNG-2025-03-17_12-39-09.webm
Second, the tree also is not responding well to mobile view. And I don't expect we can make it. (apart from making the tree vertical, which would help somewhat). To make it easy for ourselves, we could have a div with a simple text view (name the parent asset, list children and provide the button to create a child) and show that when the screen becomes narrow (in flexmeasures.css
we already use these selectors, e.g. @media (max-width: 767px) {}
)
Other than the tree, there are two issues:
- I want to simplify the map, I believe you don't need a new view and there is less things to include.
- The link to create a child should become smarter. This might mean to make the view smarter.
See my inline comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a complete review, just about the new asset improvement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the parent asset id is given, the account field should be pre-filled.
Or not, as I believe you fill it in when processing the form with a parent asset Id?
In any case, if we got an asset Id, I believe we could still show for which account we are making the asset, helps the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I press the existing new asset button (on the asset page) I get
"local variable 'parent_asset_name' referenced before assignment"
Description
Summary of the changes introduced in this PR. Try to use bullet points as much as possible.
Asset context overview
Look & Feel
How to test
To test just go to Assets listing page and click on any asset to see a high level diagramatic view of the Assets.
Further Improvements
Adding new features like listing the Asset sensors in the Asset Node.
Related Items
Closes #1350