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

Add Blueprints preview #3863

Merged
merged 25 commits into from
May 21, 2024
Merged

Add Blueprints preview #3863

merged 25 commits into from
May 21, 2024

Conversation

cstns
Copy link
Contributor

@cstns cstns commented May 16, 2024

Description

Adding support to preview blueprints using the flow-renderer across blueprint tiles

Related Issue(s)

closes #3838
closes #3875

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on FlowFuse/helm to update ConfigMap Template
    • Issue/PR raised on FlowFuse/CloudProject to update values for Staging/Production

Labels

  • Includes a DB migration? -> add the area:migration label

@cstns cstns added area:frontend For any issues that require work in the frontend/UI area:api Work on the platform API labels May 16, 2024
@cstns cstns requested a review from joepavitt May 16, 2024 07:46
@cstns cstns self-assigned this May 16, 2024
@cstns cstns removed the request for review from joepavitt May 16, 2024 08:05
Copy link

codecov bot commented May 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.27%. Comparing base (2483051) to head (ffac00c).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3863   +/-   ##
=======================================
  Coverage   79.27%   79.27%           
=======================================
  Files         281      281           
  Lines       12745    12745           
  Branches     2842     2842           
=======================================
  Hits        10104    10104           
  Misses       2641     2641           
Flag Coverage Δ
backend 79.27% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@joepavitt
Copy link
Contributor

Clicking outside dialog to close I find really annoying, as in my first attempt to create a blueprint just now, I accidentally clicked out, which closed the dialog, and I lost all progress.

Screen.Recording.2024-05-16.at.11.40.26.mov

I'm okay with this being functionality for the viewer, as there is no risk of progress-loss, but if there is a "cancel" button, I don't think click-outside should be enabled

@cstns
Copy link
Contributor Author

cstns commented May 16, 2024

It can be easily removed, I added it because I have a habit of clicking outside of dialogs to close them.

@joepavitt
Copy link
Contributor

I agree it's a useful feature, but only if the dialog doesn't have a form in place. For the v-click-outside, wrap that in a check to see if the "Cancel" button is already present, and then we can close respectively.

@joepavitt
Copy link
Contributor

joepavitt commented May 16, 2024

Also noticed this odd behaviour if we click on the zoom controls of the flow-viewer?

Screen.Recording.2024-05-16.at.13.46.05.mov

Have confirmed this only happens on the Blueprint screen, the existing flow viewer on team library and snapshots function fine

@cstns
Copy link
Contributor Author

cstns commented May 16, 2024

That's strange! I messed about with the zoom controls but not on that specific page! I'll take a look

cstns added 4 commits May 17, 2024 15:11
# Conflicts:
#	frontend/src/components/flow-viewer/SnapshotImportDialog.vue
#	frontend/src/pages/device/Snapshots/index.vue
#	frontend/src/pages/instance/Snapshots/index.vue
@Steve-Mcl
Copy link
Contributor

@cstns I see you have renamed the snapshot viewer dialog to "flow viewer" and changed the show function to accept only a flow.

It was intended this component would be able to show other parts of a snapshot in future iterations (e.g. the module list, the env vars etc).

I think a better option would be to instead add an additional show function like showBlueprint(blueprint) instead of making show(flows) less future proof.

This would mean in a future iteration, we could also support viewing the modules for blueprints modules and env vars too.

@joepavitt
Copy link
Contributor

It was intended this component would be able to show other parts of a snapshot in future iterations (e.g. the module list, the env vars etc).

We haven't implemented that functionality now, so I think the approach @cstns has taken is the correct one. It doesn't prevent any future iteration to then adapt this and include the extra points you've mentioned.

@Steve-Mcl
Copy link
Contributor

Steve-Mcl commented May 17, 2024

It was intended this component would be able to show other parts of a snapshot in future iterations (e.g. the module list, the env vars etc).

We haven't implemented that functionality now, so I think the approach @cstns has taken is the correct one. It doesn't prevent any future iteration to then adapt this and include the extra points you've mentioned.

No it doesnt, but renaming it "flow viewer" and reducing scope from what it currently is means it will need another refactoring. With up front knowledge (as provided) a 3rd rename and reverting this work can be avoided. All I am asking is to bear these point in mind.

@joepavitt
Copy link
Contributor

I think flow viewer is still appropriate here - it's more generalised than Blueprints, Library Entry or Snapshot which are the uses cases you're referring to?

@Steve-Mcl
Copy link
Contributor

I think flow viewer is still appropriate here

Not really, it will be (once we can show modules/env vars/etc) even more generic than that (more like thingViewer), but i'll stop quibbling over the name, it'll just be an under the hood implementation oddity ;)

As for reducing the parameter scope for the show function to accept only flow, I am requesting instead, rename the existing function to showSnapshot and add a new showBlueprint so that when it comes time to implement the features we don't need to undo all the changes made in this PR back to how they were & it will work for blueprints too out of the gate.

@joepavitt
Copy link
Contributor

As for reducing the parameter scope for the show function to accept only flow, I am requesting instead, rename the existing function to showSnapshot and add a new showBlueprint so that when it comes time to implement the features we don't need to undo all the changes made in this PR back to how they were & it will work for blueprints too out of the gate.

Sounds reasonable - @cstns please take action accordingly 😄

@cstns
Copy link
Contributor Author

cstns commented May 17, 2024

I'm not sure I follow. Both snapshots and blueprints have similar structures and the component's show could cover them, in which case the component name becomes problematic because it's not only a flow viewer but a more detailed viewer

@Steve-Mcl
Copy link
Contributor

I'm not sure I follow. Both snapshots and blueprints have similar structures and the component's show could cover them, in which case the component name becomes problematic because it's not only a flow viewer but a more detailed viewer

For those following along, we had a chat about this. The function parameter was named flows but really it was a thing object that contained flows so was really a none issue.

@cstns & I then agreed we can now keep a singular show function that shows the thing (be it a blueprint, snapshot, whatever) and it will be able to have modules and other future stuff in the function parameter object. All is well in the world. :)

@cstns
Copy link
Contributor Author

cstns commented May 17, 2024

We just have to find a name for that thing 🤞

@joepavitt
Copy link
Contributor

Please proceed as you had already named it @cstns - I don't want us holding up a PR over bikeshedding a name for a component

@cstns cstns requested review from joepavitt and removed request for joepavitt May 17, 2024 14:42
@joepavitt
Copy link
Contributor

Code all looks good, just updating to main as there are 14 file changes here, so want to reduce risk of a clash when merged

@joepavitt joepavitt merged commit 2884598 into main May 21, 2024
13 checks passed
@joepavitt joepavitt deleted the 3838-blueprints-preview branch May 21, 2024 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Work on the platform API area:frontend For any issues that require work in the frontend/UI deploy:pr
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Chore: Update Flow Renderer dependency to latest version 0.3.1 Blueprint preview
3 participants