Navigation Menu

Skip to content
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

Breadcrumb for the asset page #930

Merged
merged 18 commits into from Dec 20, 2023
Merged

Breadcrumb for the asset page #930

merged 18 commits into from Dec 20, 2023

Conversation

victorgarcia98
Copy link
Contributor

Description

So far, we have introduced the child-parent relationship which allows to define a hierarchy of Assets. This is very nice as to model complex relationships but currently we are unable to navigate the Asset tree through the UI.

Some time ago @GustaafL proposed to have a breadcrumb as a way to move around in FlexMeasures. This PR introduces a little breadcrumb that lets you go back to the parent asset page (if an asset has a parent) and the account page of current asset.

Look & Feel

image

How to test

  • Create a test account
flexmeasures add account --name TestAccount
  • Create a test asset type
flexmeasures add asset-type --name TestAssetType
  • Create a parent asset
flexmeasures add asset  --name Parent --account-id <account-id> --asset-type-id <asset-type-id>

flexmeasures add asset  --name Child --account-id <account-id> --asset-type-id <asset-type-id>  --parent-asset <parent-asset-id>

Further Improvements

  • Extend the breadcrumb to the sensor page

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on code under GPL or other license that is incompatible with FlexMeasures

Signed-off-by: Victor Garcia Reolid <victor@seita.nl>
@victorgarcia98 victorgarcia98 added enhancement New feature or request UI labels Dec 14, 2023
@victorgarcia98 victorgarcia98 self-assigned this Dec 14, 2023
Signed-off-by: Victor Garcia Reolid <victor@seita.nl>
Signed-off-by: Victor Garcia Reolid <victor@seita.nl>
Signed-off-by: Victor Garcia Reolid <victor@seita.nl>
@victorgarcia98 victorgarcia98 marked this pull request as ready for review December 14, 2023 15:35
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice addition!

One caveat: I guess we can have more than one parental layer.

E.g. Asset1 <- Asset2 <- Asset3

I believe this would then swallow the highest asset and only show:

Account - Asset2 - Asset3

Maybe we can update this, so parent_asset becomes parent_assets ?

And extending to sensors would be great. I believe that is simpler (one asset per sensor), so that should use a big part of this code.
Please make an extra issue or include right here.

Copy link
Contributor

@Flix6x Flix6x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a code review. Testing is next.

flexmeasures/cli/data_add.py Outdated Show resolved Hide resolved
flexmeasures/ui/crud/assets.py Outdated Show resolved Hide resolved
flexmeasures/ui/crud/assets.py Outdated Show resolved Hide resolved
flexmeasures/ui/crud/assets.py Outdated Show resolved Hide resolved
flexmeasures/ui/crud/assets.py Outdated Show resolved Hide resolved
flexmeasures/ui/crud/assets.py Outdated Show resolved Hide resolved
@Flix6x
Copy link
Contributor

Flix6x commented Dec 15, 2023

A couple more thoughts I had (just writing them down before I forget):

  • For a given asset, are all of its parents guaranteed to belong to the same account?
  • We could use an ellipsis to leave out intermediate parents (account > grandparent A > ... > parent F > child), as we can't show parents indefinitely.
  • Let's check responsiveness on different screen sizes.
  • Advanced feature idea: open a menu list of siblings on hover (see MSDN example)

@victorgarcia98
Copy link
Contributor Author

One caveat: I guess we can have more than one parental layer.

True, now the function returns all the parental layers. A follow-up feature might be to limit the number of parental layers.

And extending to sensors would be great. I believe that is simpler (one asset per sensor), so that should use a big part of this code. Please make an extra issue or include right here.

Good point, It's sometime hard to know which sensor you are looking and to what simulation it belongs. At the moment, we are looking at the tooltip an sensor name.

Signed-off-by: Victor Garcia Reolid <victor@seita.nl>
@victorgarcia98
Copy link
Contributor Author

A couple more thoughts I had (just writing them down before I forget):

* For a given asset, are all of its parents guaranteed to belong to the same account?

Currently, the parents are not guaranteed to belong to the same account.

* We could use an ellipsis to leave out intermediate parents (`account > grandparent A > ... > parent F > child`), as we can't show parents indefinitely.

Good point. Nonetheless, the number of parent layers will be pretty limited compared to the number of assets.

* Let's check responsiveness on different screen sizes.

I've resized my window and it looks good to me. In that case, I see the text jumping to the next line.

* Advanced feature idea: open a menu list of siblings on hover ([see MSDN example](https://www.smashingmagazine.com/2009/03/breadcrumbs-in-web-design-examples-and-best-practices/))

This would be very nice. Also, we could think on navigating lower levels. For example, from an asset, list all the sensors and/or children.

flexmeasures/ui/templates/crud/asset.html Outdated Show resolved Hide resolved
flexmeasures/ui/utils/view_utils.py Outdated Show resolved Hide resolved
flexmeasures/ui/crud/assets.py Show resolved Hide resolved
flexmeasures/ui/utils/breadcrumb_utils.py Outdated Show resolved Hide resolved
flexmeasures/ui/utils/breadcrumb_utils.py Outdated Show resolved Hide resolved
Signed-off-by: Victor Garcia Reolid <victor@seita.nl>
@nhoening nhoening added this to the 0.18.0 milestone Dec 18, 2023
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Copy link
Contributor

@Flix6x Flix6x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, with two asks:

  1. "breadcrumb": get_ancestry(entity), -> change key to "ancestors"
  2. changelog entry

(Note that I added a bit of CSS styling.)

Signed-off-by: Victor Garcia Reolid <victor@seita.nl>
Signed-off-by: Victor <victor@seita.nl>
Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool, thanks.

Just one line that comes from a different PR and slipped through merging I guess.

documentation/changelog.rst Outdated Show resolved Hide resolved
Signed-off-by: Victor Garcia Reolid <victor@seita.nl>
@nhoening nhoening merged commit a646fed into main Dec 20, 2023
9 checks passed
@nhoening nhoening deleted the feature/ui/breadcrumb branch December 20, 2023 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants