-
Notifications
You must be signed in to change notification settings - Fork 384
share: Support switching accounts #1883
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
Conversation
chrisbobbe
left a comment
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.
Thanks! Comments below.
| SubscriptionListPageBody( | ||
| showTopicListButtonInActionSheet: false, | ||
| hideChannelsIfUserCantSendMessage: true, | ||
| allowGoToAllChannels: false, |
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.
This removal looks unintentional.
lib/widgets/share.dart
Outdated
| // We should already have the `store.realmIcon` after the | ||
| // PerAccountStore has completed loading, hence the `!` here. | ||
| final realmIconUrl = store.realmUrl.resolveUri(store.realmIcon!); |
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.
If a PerAccountStore is guaranteed to have non-null .realmIcon, then why is it typed as optional? If we can make it required, we should do so, right? (Ditto .realmName.) Then if there's some subtlety in explaining that, we can do it just once where the field is declared, instead of all the places that try to consume it.
lib/widgets/share.dart
Outdated
| SizedBox.square( | ||
| dimension: 42, | ||
| child: Padding( | ||
| padding: const EdgeInsets.all(7), | ||
| child: RealmContentNetworkImage(realmIconUrl))), |
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.
Following the link to the "company-logo" component in Figma—
https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=421-14693&m=dev
—it looks like this is supposed to have a 4px border radius:
Would the AvatarShape widget be appropriate here? Its dartdoc says "A rounded square shape, to wrap an [AvatarImage] or similar.".
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 also see an additional 4px horizontal padding in the Figma that's not implemented in this revision:
https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=12853-76635&m=dev
| labelStyle: labelStyle, | ||
| labelColor: designVariables.iconSelected, | ||
| unselectedLabelColor: designVariables.icon, | ||
| indicatorWeight: 0, |
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.
Hmm, two things stand out to me in the dartdoc of indicatorWeight:
/// The value of this parameter must be greater than zero./// If [indicator] is specified […] this property is ignored.
What happens if we don't pass indicatorWeight?
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.
Added a bit of context in a comment on why the indicatorWeight: 0 is needed here.
lib/widgets/share.dart
Outdated
| final labelStyle = TextStyle( | ||
| fontSize: 18, | ||
| height: 24 / 18, | ||
| letterSpacing: 0, |
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.
Isn't letterSpacing: 0 the default, from zulipTypography?
lib/widgets/share.dart
Outdated
| child: RealmContentNetworkImage(realmIconUrl))), | ||
| ), |
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.
| child: RealmContentNetworkImage(realmIconUrl))), | |
| ), | |
| child: RealmContentNetworkImage(realmIconUrl)))), |
lib/widgets/share.dart
Outdated
| ShareDialog.show( | ||
| pageContext: context, | ||
| initialAccountId: accountId, | ||
| sharedFiles: sharedFiles, | ||
| sharedText: intentSendEvent.extraText); |
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.
share: Re-design from page to a dialog
This commit breaks the "New DM" button in the "Direct messages" tab, because a PageRoot ancestor is no longer found when the button is tapped:
======== Exception caught by gesture ===============================================================
The following assertion was thrown while handling a gesture:
No PageRoot ancestor
'package:zulip/widgets/page.dart':
Failed assertion: line 26 pos 12: 'element != null'
When the exception was thrown, this was the stack:
#2 PageRoot.contextOf (package:zulip/widgets/page.dart:26:12)
#3 showNewDmSheet (package:zulip/widgets/new_dm_sheet.dart:17:32)
#4 _NewDmButtonState.build.<anonymous closure> (package:zulip/widgets/recent_dm_conversations.dart:270:20)
[etc.]
lib/widgets/share.dart
Outdated
| unawaited(() async { | ||
| final GetServerSettingsResult serverSettings; | ||
| final connection = globalStore.apiConnection( | ||
| realmUrl: account.realmUrl, | ||
| zulipFeatureLevel: null); | ||
| try { | ||
| serverSettings = await getServerSettings(connection); | ||
| } catch (_) { | ||
| return; | ||
| } finally { | ||
| connection.close(); | ||
| } | ||
|
|
||
| if (globalStore.getAccount(accountId) != null) { | ||
| await globalStore.updateRealmData( | ||
| accountId, | ||
| realmName: serverSettings.realmName, | ||
| realmIcon: serverSettings.realmIcon); | ||
| } | ||
| }()); |
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.
The goal is to make sure the realm name and icon are present and up-to-date, right?
I think my ideal way to do this would be more centralized and a bit more rigorous:
- The choose-account page/dialog doesn't yet show the name and icon, but it will, and it'll be equally useful there to refresh them as here.
- We've written careful "refuse-to-connect-to-ancient-servers" logic that should eliminate ancient-server behavior as a possible source of bugs, which should help simplify debugging. This revision puts a small hole in that logic, which we could fix by checking
ZulipVersionData.isUnsupportedbefore interpretingrealmNameandrealmIcon. Like we do around the othergetServerSettingscall.- I don't think any ancient servers format
realmNameandrealmIconin weird, interfering ways…but the point is being confident that it wouldn't matter even if they did.
- I don't think any ancient servers format
- The realm name and icon are also provided via the event system, and we should defer to that: if a
PerAccountStorefinishes loading before we've received and recorded the data from get-server-settings, we should abort instead of clobbering the store with this data that arrived out-of-band.
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.
more centralized
I'm not sure yet where's the most helpful place to put this. Maybe in a stateful widget that gets passed to PerAccountStoreWidget.placeholder? I'd be happy to think about that when I revisit my draft commits in #1816, and in the meantime perhaps you can just address my second and third bullet points above, which are correctness issues.
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 think a good home for this would be as a method on GlobalStore, perhaps named like ensureFreshAccountData. (Or ensureFreshRealmMetadata? The point is it's the data (a) that we store in the Account record but (b) about the realm as a whole.)
Then that method will have access to _perAccountStoresLoading as well as _perAccountStores. It can look at those in order to ensure that it doesn't do anything if there's already a PerAccountStore or one being loaded.
| return const SizedBox.shrink(); | ||
| } | ||
|
|
||
| return ListTile( |
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.
Perhaps add a TODO(#1038) for aligning this dialog with other choose-account dialogs (link: #1038)
lib/widgets/share.dart
Outdated
| onTap: hasMultipleAccounts | ||
| ? () { | ||
| ChooseAccountForShareDialog.show( | ||
| pageContext: context, | ||
| selectedAccountId: store.accountId, | ||
| sharedFiles: sharedFiles, | ||
| sharedText: sharedText); | ||
| } |
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.
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 dunno, at least as an Android user, I feel like one is pretty well trained that clicking on your avatar opens the account switcher.
|
@alya curious for your thoughts on #1883 (comment) too. |
|
@rajveermalviya gentle bump on this; there's material from my draft commits in #1816 that I'd like to refresh and send as a PR after this PR settles. |
01bc09d to
db3de4b
Compare
|
I see you've pushed changes but I'm not sure if you're intending them to be ready for review 🙂 please let me know when ready. |
f02fe4a to
c02e265
Compare
|
Thanks for the review @chrisbobbe! Sorry for the delay. I've pushed a new revision, PTAL. |
79ceed6 to
955d505
Compare
chrisbobbe
left a comment
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.
Thanks! Small comments below.
lib/model/store.dart
Outdated
| /// to update the realm metadata (name and icon). | ||
| /// | ||
| /// The updation for some accounts may be skipped if: | ||
| /// - The per account store for the account is already loaded or is loading. |
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.
nit: Helpful to use the identifier [PerAccountStore] here
lib/model/store.dart
Outdated
| /// - The version of corresponding realm server is unsupported. | ||
| /// | ||
| /// Returns immediately; the fetches and updates are done asynchronously. | ||
| void ensureFreshRealmMetadata() { |
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.
This name makes me think that…well, that it'll ensure the realm metadata is fresh 🙂, and that it'll accomplish that before it returns. (I guess I'm thinking of calls like TestZulipBinding.ensureInitialized();.)
Really, it just kicks off an attempt at refreshing the metadata, under certain conditions (like that the server isn't ancient); an attempt which may or may not succeed, sometime after the function returns.
How about tryRefreshRealmMetadata, or just refreshRealmMetadata? I think the latter is fine (it's shorter); I think the important improvement is just to remove the really strong "ensure" wording.
lib/widgets/share.dart
Outdated
| child: PageRoot( | ||
| child: ShareSheet( | ||
| sharedFiles: sharedFiles, | ||
| sharedText: sharedText), | ||
| )); |
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.
nit: parens at end
lib/widgets/share.dart
Outdated
| text, | ||
| overflow: TextOverflow.ellipsis, | ||
| maxLines: 1, | ||
| style: TextStyle( |
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.
nit: extra space
lib/widgets/share.dart
Outdated
| splashColor: Colors.transparent, | ||
| leading: AspectRatio( | ||
| aspectRatio: 1, | ||
| child: account.realmIcon == null |
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.
Isn't the account.realmIcon == null case impossible here, because of the account.realmIcon! above?
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.
Ahh, the account.realmIcon! above should have a conditional, because account.{realmIcon,realmName} == null is possible for accounts that never had the realm metadata stored. i.e PerAccountStore wasn't loaded recently (specifically after a8b236d) or GlobalStore.refreshRealmMetadata hasn't updated the data yet.
lib/widgets/share.dart
Outdated
| sharedText: widget.sharedText); | ||
| }, | ||
| splashColor: Colors.transparent, | ||
| leading: AspectRatio( |
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.
How about using AvatarShape for the realm icon, as elsewhere
| headers: userAgentHeader(), | ||
| filterQuality: FilterQuality.medium, | ||
| fit: BoxFit.cover)), | ||
| title: Text(account.realmName ?? account.realmUrl.toString()), |
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.
Can account.realmName ever be null here?
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.
Yes, see: #1883 (comment)
lib/widgets/share.dart
Outdated
| header: Padding( | ||
| padding: const EdgeInsets.only(top: 8), | ||
| child: BottomSheetHeader(title: zulipLocalizations.shareChooseAccountLabel)), |
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.
Instead of the Padding widget, pass outerVerticalPadding: true to BottomSheetHeader. (I think this was an API change in BottomSheetHeader that landed after your first revision of this.)
3c9cc57 to
5d559ae
Compare
|
Thanks for the review @chrisbobbe! Pushed an update, PTAL. |
|
Thanks! Marking for Greg's review. Thanks for the reminder at #1883 (comment) — AIUI that the realm icon/name will be present on a |
gnprice
left a comment
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.
Thanks @rajveermalviya, and thanks @chrisbobbe for the previous reviews!
Generally this looks good; comments below.
I haven't yet looked at the screenshots or tried experimenting with the UI.
lib/model/store.dart
Outdated
| /// Fetches server settings for each account and then calls [updateRealmData] | ||
| /// to update the realm metadata (name and icon). | ||
| /// | ||
| /// The updation for some accounts may be skipped if: |
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.
nit: "updation" isn't a word 🙂; could say "the update"
lib/model/store.dart
Outdated
| /// | ||
| /// The updation for some accounts may be skipped if: | ||
| /// - The [PerAccountStore] for the account is already loaded or is loading. | ||
| /// - The version of corresponding realm server is unsupported. |
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 can't quite parse this last line. Does this cover what you mean?
| /// - The version of corresponding realm server is unsupported. | |
| /// - The Zulip Server version of the realm's server is unsupported. |
| // Avoid updating realm metadata if per account store has already | ||
| // loaded or is being loaded. It should be updated with data from | ||
| // the initial snapshot or realm update events. | ||
| if (_perAccountStoresLoading.containsKey(accountId)) continue; | ||
| if (_perAccountStores.containsKey(accountId)) continue; |
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.
Here "should" sounds to me like it's saying there's some obligation which has yet to be discharged. I think what this is intended to say is a reassurance: the update will get taken care of, by other code.
| // Avoid updating realm metadata if per account store has already | |
| // loaded or is being loaded. It should be updated with data from | |
| // the initial snapshot or realm update events. | |
| if (_perAccountStoresLoading.containsKey(accountId)) continue; | |
| if (_perAccountStores.containsKey(accountId)) continue; | |
| // Avoid updating realm metadata if per account store has already | |
| // loaded or is being loaded. It will be updated with data from | |
| // the initial snapshot or realm update events. | |
| if (_perAccountStoresLoading.containsKey(accountId)) continue; | |
| if (_perAccountStores.containsKey(accountId)) continue; |
lib/model/store.dart
Outdated
| final account = getAccount(accountId); | ||
| if (account == null) continue; |
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.
What's the situation where this condition can arise?
… I think it can't — this is a lookup in the same map that we're iterating through the keys of. So let's write the logic in a way that makes explicit that this case is impossible.
An immediate version of that would be to say ! here — that makes clear we believe it's impossible. Better yet: iterate through accountEntries rather than accountIds, so that this case is structurally impossible by the way the logic is arranged.
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.
Or for that matter iterate through accounts, and use account.id to get the account ID.
lib/model/store.dart
Outdated
| final GetServerSettingsResult serverSettings; | ||
| final connection = apiConnection( | ||
| realmUrl: account.realmUrl, | ||
| zulipFeatureLevel: null); | ||
| try { | ||
| serverSettings = await getServerSettings(connection); | ||
| final zulipVersionData = ZulipVersionData.fromServerSettings(serverSettings); | ||
| if (zulipVersionData.isUnsupported) { | ||
| throw ServerVersionUnsupportedException(zulipVersionData); | ||
| } | ||
| } on MalformedServerResponseException catch (e) { | ||
| final zulipVersionData = ZulipVersionData.fromMalformedServerResponseException(e); | ||
| if (zulipVersionData != null && zulipVersionData.isUnsupported) { | ||
| throw ServerVersionUnsupportedException(zulipVersionData); | ||
| } | ||
| rethrow; | ||
| } finally { | ||
| connection.close(); | ||
| } |
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.
This logic is rather subtle, and it's duplicated with the other place we call getServerSettings, in the login flow. Can we deduplicate? Something like this:
| final GetServerSettingsResult serverSettings; | |
| final connection = apiConnection( | |
| realmUrl: account.realmUrl, | |
| zulipFeatureLevel: null); | |
| try { | |
| serverSettings = await getServerSettings(connection); | |
| final zulipVersionData = ZulipVersionData.fromServerSettings(serverSettings); | |
| if (zulipVersionData.isUnsupported) { | |
| throw ServerVersionUnsupportedException(zulipVersionData); | |
| } | |
| } on MalformedServerResponseException catch (e) { | |
| final zulipVersionData = ZulipVersionData.fromMalformedServerResponseException(e); | |
| if (zulipVersionData != null && zulipVersionData.isUnsupported) { | |
| throw ServerVersionUnsupportedException(zulipVersionData); | |
| } | |
| rethrow; | |
| } finally { | |
| connection.close(); | |
| } | |
| final serverSettings = await fetchServerSettings(account.realmUrl); |
We'd arrange that as an NFC commit factoring out GlobalStore.fetchServerSettings (and using it in the existing place), then the commit adding this refreshRealmMetadata method using it.
lib/model/store.dart
Outdated
| if (account == null) continue; | ||
|
|
||
| // Fetch the server settings and update the realm data without awaiting. | ||
| // This allows fetching server settings of all the accounts parallelly. |
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.
nit: "parallelly" isn't a word
| // This allows fetching server settings of all the accounts parallelly. | |
| // This allows fetching server settings of all the accounts in parallel. |
lib/model/store.dart
Outdated
| final zulipVersionData = ZulipVersionData.fromServerSettings(serverSettings); | ||
| if (zulipVersionData.isUnsupported) { | ||
| throw ServerVersionUnsupportedException(zulipVersionData); | ||
| } |
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.
This part seems unnecessary. If we can successfully parse the name and icon, why not use them?
Happily I think this also belongs outside the scope of a "fetch server settings" helper method. The other call site of that method can do this part itself after calling the method.
lib/widgets/share.dart
Outdated
| Expanded(child: TabBar( | ||
| padding: EdgeInsets.zero, |
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.
| Expanded(child: TabBar( | |
| padding: EdgeInsets.zero, | |
| Expanded(child: TabBar( |
This is equivalent, right?
lib/widgets/share.dart
Outdated
| final globalStore = GlobalStoreWidget.of(context); | ||
| final zulipLocalizations = ZulipLocalizations.of(context); | ||
|
|
||
| final accountIds = List<int>.unmodifiable(globalStore.accountIds); |
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.
nit: this may as well get the list of actual account objects:
| final accountIds = List<int>.unmodifiable(globalStore.accountIds); | |
| final accounts = List<int>.unmodifiable(globalStore.accounts); |
That simplifies the code below a bit, by eliminating any wonder about if the accounts can go missing.
This version will update itself the same way, in the end, that the current revision does: this widget is already registering a dependency on GlobalStoreWidget, so it will get rebuilt when the GlobalStore notifies its listeners, which happens whenever the list of accounts (or any individual account within it) changes.
5d559ae to
6fd5a6b
Compare
6fd5a6b to
876dcdc
Compare
|
Thanks for the review @gnprice! Pushed an update, PTAL. |
gnprice
left a comment
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.
Thanks for the revision! Generally this all looks good, and I've also now gone through the screenshots and tried out the UI. Small comments below.
lib/model/store.dart
Outdated
| final serverSettings = await getServerSettings(connection); | ||
| return serverSettings; |
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.
nit: simplify
lib/widgets/share.dart
Outdated
|
|
||
| return ListTile( | ||
| onTap: () { | ||
| if (globalStore.getAccount(accountId) == null) return; |
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.
What's the situation where this could occur?
I think it can't, for the reasons at #1883 (comment) .
lib/widgets/share.dart
Outdated
| } | ||
| } | ||
|
|
||
| class ChooseAccountForShareDialog extends StatefulWidget { |
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.
This isn't a dialog — it's a bottom sheet. I think s/Dialog/BottomSheet/ would do.
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.
Or s/Dialog/Modal/. That's a more general category that includes dialogs and modal bottom sheets, and maybe it's the more apt level of abstraction to think about the role of this piece of UI. (It also has a shorter name.)
| static void show({ | ||
| required BuildContext pageContext, | ||
| required int selectedAccountId, | ||
| required Iterable<FileToUpload>? sharedFiles, | ||
| required String? sharedText, | ||
| }) async { | ||
| unawaited(showModalBottomSheet<void>( |
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 like this organization where the "show" function is a static method on the widget class. That helps keep the widget code organized into widget classes, like it is for most everything other than dialogs and bottom sheets.
lib/widgets/share.dart
Outdated
| } | ||
|
|
||
| class ChooseAccountForShareDialog extends StatefulWidget { | ||
| const ChooseAccountForShareDialog({ |
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.
The one downside of this way of organizing it (with "show" as a static method on the widget class) is that it makes the widget class public, when it's really an implementation detail of the "show" function.
I think that can be addressed nicely, though, by making the constructor here private. That clarifies that the widget is only meant to be used via the "show" method. (And it's hardly a surprise that there will be some sort of widget corresponding to the modal; so with the constructor private, the class itself being public is harmless.)
assets/l10n/app_en.arb
Outdated
| "description": "Title for the page about sharing content received from other apps." | ||
| "shareChooseAccountLabel": "Choose an account", | ||
| "@shareChooseAccountLabel": { | ||
| "description": "Label for the page about selecting an account to share content received from other apps." |
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.
The thing this is used for isn't a page; it's a modal bottom sheet.
lib/widgets/share.dart
Outdated
|
|
||
| return DraggableScrollableModalBottomSheet( | ||
| header: BottomSheetHeader( | ||
| title: zulipLocalizations.shareChooseAccountLabel, |
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.
nit: Seems like "title" would be a more apt term than "label" in the name of this string.
28f32dc to
b9c02f1
Compare
|
Thanks for the review @gnprice! Pushed an update, PTAL. |
Extract some of the complexity around `getServerSettings` in the login flow, into a helper method in `GlobalStore`.
A helper that attempts to update the realm metadata (name and icon) for each account.
Co-Authored-By: Chris Bobbe <[email protected]> Fixes: zulip#1779
|
Thanks! All looks good; merging. |
b9c02f1 to
f813bc8
Compare







Stacked on top of ea5cd48 and b8e0143 from #1937.
Separated from #1816 to preserve some drafts commits there from @chrisbobbe.
Screenshots coming..
Tests are TODO and will happen as part of #1787 in a follow-up PR.
Figma: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=12853-76543&p=f&t=oBRXWxFjbkz1yeI7-0
Fixes: #1779