-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
feat(mobile): dynamic layout in new timeline #23837
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import 'package:easy_localization/easy_localization.dart'; | ||
| import 'package:flutter/material.dart'; | ||
| import 'package:hooks_riverpod/hooks_riverpod.dart'; | ||
| import 'package:immich_mobile/entities/store.entity.dart'; | ||
| import 'package:immich_mobile/extensions/translate_extensions.dart'; | ||
| import 'package:immich_mobile/providers/app_settings.provider.dart'; | ||
| import 'package:immich_mobile/services/app_settings.service.dart'; | ||
|
|
@@ -24,11 +25,12 @@ class LayoutSettings extends HookConsumerWidget { | |
| title: "asset_list_layout_sub_title".t(context: context), | ||
| icon: Icons.view_module_outlined, | ||
| ), | ||
| SettingsSwitchListTile( | ||
| valueNotifier: useDynamicLayout, | ||
| title: "asset_list_layout_settings_dynamic_layout_title".t(context: context), | ||
| onChanged: (_) => ref.invalidate(appSettingsServiceProvider), | ||
| ), | ||
| if (!Store.isBetaTimelineEnabled) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to be what causes #27129 - the setting previously appeared on both timelines, despite being only supported by the old one. I guess that this was the initial attempt here - just removing the unsupported setting without adding support for the new timeline? Since this is fixed in the same PR, having this change left in here is certainly a bug.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reverting this change makes the toggle show up, but the dynamic layout still doesn't work. |
||
| SettingsSwitchListTile( | ||
| valueNotifier: useDynamicLayout, | ||
| title: "asset_list_layout_settings_dynamic_layout_title".t(context: context), | ||
| onChanged: (_) => ref.invalidate(appSettingsServiceProvider), | ||
| ), | ||
| SettingsSliderListTile( | ||
| valueNotifier: tilesPerRow, | ||
| text: 'theme_setting_asset_list_tiles_per_row_title'.tr(namedArgs: {'count': "${tilesPerRow.value}"}), | ||
|
|
||
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 this be a const constructor?
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.
Since we have varying widths passed to this, the object can never be const, even when we have a const constructor for it. Having the factory here at-least makes it easier to create such objects