Reorganize data-managing widgets to accommodate global data #20
Conversation
lib/widgets/app.dart
Outdated
| @override | ||
| Widget build(BuildContext context) { | ||
| if (store == null) return const LoadingPage(); | ||
| return PerAccountStoreWidget( | ||
| store: store!, | ||
| child: MaterialApp( | ||
| title: 'Zulip', | ||
| theme: ThemeData( | ||
| // This applies Material 3's color system to produce a palette of | ||
| // appropriately matching and contrasting colors for use in a UI. | ||
| // The Zulip brand color is a starting point, but doesn't end up as | ||
| // one that's directly used. (After all, we didn't design it for that | ||
| // purpose; we designed a logo.) See docs: | ||
| // https://api.flutter.dev/flutter/material/ColorScheme/ColorScheme.fromSeed.html | ||
| // Or try this tool to see the whole palette: | ||
| // https://m3.material.io/theme-builder#/custom | ||
| colorScheme: ColorScheme.fromSeed(seedColor: kZulipBrandColor)), | ||
| home: const HomePage(), | ||
| )); | ||
| return PerAccountStoreWidget(store: store!, child: widget.child); | ||
| } |
There was a problem hiding this comment.
widgets/app [nfc]: Pull UI out of data-management code
After this commit, there's still one more bit of UI being directed by _PerAccountRootState: the LoadingPage() that shows when store == null. Should we pull that out too somehow? I sense that you want to use PerAccountRoot in places where a "page" won't be the right thing to build. 🙂
There was a problem hiding this comment.
Good point, yeah. I had that thought but then forgot it. I'll leave a TODO.
| // TODO add a registry of [PerAccountStore]s, like the latter's of [MessageListView] | ||
| // That will allow us to have many [PerAccountRoot] widgets for a given | ||
| // account, e.g. at the top of each page; and to access server data from | ||
| // outside any [PerAccountRoot], e.g. for handling a notification. |
There was a problem hiding this comment.
many [PerAccountRoot] widgets for a given account, e.g. at the top of each page
This seems like a case where PerAccountRoot's instruction to build a "loading page" could be problematic. Until the store is available, each page will have a loading page at the top?
I don't yet understand why we'll want to have multiple PerAccountRoots per account. In what sense is each one a "root", then? 🤔
There was a problem hiding this comment.
Until the store is available, each page will have a loading page at the top?
Hmm, I suppose there's more for me to think through on that front. (But this is a TODO describing #21, not really part of what this PR itself does — so I think it needn't block merging this one.)
In what sense is each one a "root", then? 🤔
I think as part of this the name will change to not say "root".
I don't yet understand why we'll want to have multiple
PerAccountRoots per account.
Does #21 explain it? If not, let's discuss there.
|
Thanks for the review! Merging, after in-person discussion on the PerAccountRoot question above. |
Another step toward #13 . This sets up an appropriate home (the
GlobalStore) for data like the list of accounts, and which one is the active account (the one we should immediately show on launching the app), to live.