-
Notifications
You must be signed in to change notification settings - Fork 63
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
Snapshot import export component options #4610
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4610 +/- ##
==========================================
+ Coverage 78.28% 78.34% +0.06%
==========================================
Files 303 302 -1
Lines 14367 14390 +23
Branches 3267 3283 +16
==========================================
+ Hits 11247 11274 +27
+ Misses 3120 3116 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@joepavitt added as reviewer for frontend. This was rejigged slightly from the mock up screens agreed in the issue - in that the The reason being: When toggling the component option "credentials", the component options group were jumping up and down the dialog due to toggling the visibility of the |
…com/FlowFuse/flowfuse into 4605-snapshot-import-export-options
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.
UX-wise, happy with it all. One comment on the instruction in the "download" dialog. Functionally, seems to work too, although front-end E2E's should be added, as this is a risky area that will definitely impact user's production environments
frontend/src/pages/application/Snapshots/components/dialogs/SnapshotExportDialog.vue
Outdated
Show resolved
Hide resolved
…apshotExportDialog.vue Co-authored-by: Joe Pavitt <99246719+joepavitt@users.noreply.github.com>
Yeah, absolutely makes sense & agree. Will add E2E tests. |
@joepavitt e2e tests added That was not easy - but definitely worth the effort as it turned up a couple of edge cases (like re-selecting same snapshot to upload would not work) NOTE: There were a number of challenges in implementing the e2e tests for multiple downloads/uploads tests! One particular puzzling doozy was with uploading fixtures that several other users of cypress have faced over the years (and a hand full of "me too" issues on cypress issue tracker - none of which worked here!). Essentially, fingers crossed they pas on GH too 🤞 |
@joepavitt FYI |
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.
I'd already said it was all good in a comment, just hadn't clicked the Approve button/ With Nick's approval, you could have just gone ahead, but anyway, my explicit approval is given.
closes #4605
Description
Potential clean up: move
SnapshotExportDialog.vue
to be more reusable, sitting along sidefrontend/src/components/dialogs/SnapshotImportDialog.vue
Unit Tests added
e2e tests added
TODO:
Related Issue(s)
#4605
Checklist
flowforge.yml
?FlowFuse/helm
to update ConfigMap TemplateFlowFuse/CloudProject
to update values for Staging/ProductionLabels
area:migration
label