Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6945 +/- ##
=======================================
Coverage 76.29% 76.29%
=======================================
Files 403 403
Lines 20296 20296
Branches 4881 4881
=======================================
Hits 15485 15485
Misses 4811 4811
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cstns
left a comment
cstns
left a comment
There was a problem hiding this comment.
Awesome first step in migrating the account store to Pinia!
I’ve raised a few points that will better position us for upcoming changes and create a more resilient organization
| export const useAccountAuthStore = defineStore('account-auth', { | ||
| state: () => ({ | ||
| user: null, | ||
| pending: true, |
There was a problem hiding this comment.
The pending key currently toggles the application's main loading overlay, so I don't think it belongs in the account store.
Since we’re already slicing the account store, this is the perfect time to extract this into a dedicated store. Given it’s purely UI/UX logic, it should be namespaced accordingly. I’m leaning toward ux-loading, but I'm open to other names.
The current key names also don’t quite reflect their purpose. In preparation for upcoming page and component-level loaders, I suggest a more scalable structure within a new ux-loading store:
- global (renamed from pending)
- network (renamed from offline)
- page (upcoming)
- components (upcoming)
These aren’t set in stone, but grouping them this way or similar fashion feels more intuitive for the long term.
TL;DR: Let's extract the pending state and ancillary loading functionality into a distinct store under the ux namespace and fight me on the naming
There was a problem hiding this comment.
Separated out into new ux-loading store with the following name changes:
pending->appLoaderoffline->offline- kept this as is since network is confusing when set to true, implies we are online.
cstns
left a comment
cstns
left a comment
There was a problem hiding this comment.
Tested locally and everything looks good except the comment above which I'd like to talk about when you get in
Nice job!
|
ok, that's what was unclear to me. GTG then! |
2 participants
Description
See details & test plan
Todos
Related Issue(s)
Resolves #6829
Checklist
flowforge.yml?FlowFuse/helmto update ConfigMap TemplateFlowFuse/CloudProjectto update values for Staging/ProductionLabels
area:migrationlabel