-
Notifications
You must be signed in to change notification settings - Fork 31
feat: Implement tab grouping functionality #1719
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
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.
Pull request overview
This pull request implements tab grouping functionality across all three platforms (iOS, Android, and Desktop), allowing users to organize multiple timeline tabs into named groups with custom icons. This addresses issue #1690.
Changes:
- Added
GroupConfigScreento all three platforms (iOS, Android, Desktop) for creating and editing tab groups - Modified
TabSettingsScreenacross all platforms to support creating, editing, and managing tab groups alongside individual tabs - Added localization strings for group-related UI elements across all platforms
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| iosApp/flare/UI/Screen/TabSettingsScreen.swift | Updated to handle both regular tabs and groups, added menu for choosing between adding tabs or groups |
| iosApp/flare/UI/Screen/GroupConfigScreen.swift | New screen for configuring tab groups with name, icon, and child tabs |
| iosApp/flare/UI/Route/Route.swift | Added route case for tab group configuration |
| iosApp/flare/Localizable.xcstrings | Added localization keys for group functionality |
| desktopApp/src/main/kotlin/dev/dimension/flare/ui/screen/home/TabSettingScreen.kt | Updated to display menu for adding groups or tabs |
| desktopApp/src/main/kotlin/dev/dimension/flare/ui/screen/home/GroupConfigScreen.kt | New screen implementing group configuration with proper localization |
| desktopApp/src/main/kotlin/dev/dimension/flare/ui/route/Router.kt | Added routing for group configuration screen |
| desktopApp/src/main/kotlin/dev/dimension/flare/ui/route/Route.kt | Added route data class for tab group configuration |
| desktopApp/src/main/composeResources/values/strings.xml | Added complete set of localization strings for group functionality |
| app/src/main/res/values/strings.xml | Added partial localization strings for group functionality (missing some keys) |
| app/src/main/java/dev/dimension/flare/ui/screen/settings/TabCustomizeScreen.kt | Added isEditing parameter to highlight currently edited tab |
| app/src/main/java/dev/dimension/flare/ui/screen/home/TabSettingScreen.kt | Updated to support group editing and added dropdown menu |
| app/src/main/java/dev/dimension/flare/ui/screen/home/GroupConfigScreen.kt | New screen for group configuration with some hardcoded strings |
| app/src/main/java/dev/dimension/flare/ui/screen/home/HomeEntryBuilder.kt | Added routing entry for group configuration |
| app/src/main/java/dev/dimension/flare/ui/route/Route.kt | Added route for tab group configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .navigationTitle("Select Icon") | ||
| .toolbar { | ||
| ToolbarItem(placement: .cancellationAction) { | ||
| Button("Cancel") { |
Copilot
AI
Jan 20, 2026
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 title "Select Icon" and button text "Cancel" are hardcoded strings instead of using localized string keys. These should use localized strings for proper internationalization support.
| .navigationTitle("Select Icon") | |
| .toolbar { | |
| ToolbarItem(placement: .cancellationAction) { | |
| Button("Cancel") { | |
| .navigationTitle(NSLocalizedString("Select Icon", comment: "Title for icon selection screen")) | |
| .toolbar { | |
| ToolbarItem(placement: .cancellationAction) { | |
| Button(NSLocalizedString("Cancel", comment: "Cancel icon selection")) { |
app/src/main/java/dev/dimension/flare/ui/screen/home/GroupConfigScreen.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/dev/dimension/flare/ui/screen/home/GroupConfigScreen.kt
Outdated
Show resolved
Hide resolved
| subTimelineTabItem = tabs.toList(), | ||
| metaData = | ||
| TabMetaData( | ||
| title = TitleType.Text(name.text.toString().ifEmpty { "Group" }), |
Copilot
AI
Jan 20, 2026
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 default group name "Group" is hardcoded instead of using a localized string resource. This should use a string resource for consistency with the desktop implementation which uses tab_settings_group_default_name. This hardcoded value will not be translated for non-English users.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot fix the build by |
Co-authored-by: Tlaster <8370277+Tlaster@users.noreply.github.com>
Fix ktlint line length violation in GroupConfigScreen.kt
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.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <string name="tab_settings_edit_group">Edit Group</string> | ||
| <string name="tab_settings_group_empty">No tabs in this group</string> | ||
| <string name="tab_settings_group_name_placeholder">Group name</string> | ||
| <string name="tab_settings_group_default_name">New Group</string> |
Copilot
AI
Jan 21, 2026
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.
Inconsistent default group name across platforms. Android uses "New Group" while iOS and desktop use "Group". This inconsistency could confuse users switching between platforms.
| <string name="tab_settings_group_default_name">New Group</string> | |
| <string name="tab_settings_group_default_name">Group</string> |
| subTimelineTabItem = tabs.toList(), | ||
| metaData = | ||
| TabMetaData( | ||
| title = TitleType.Text(name.text.toString().ifEmpty { "Group" }), |
Copilot
AI
Jan 21, 2026
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.
Missing default group name usage. Unlike the iOS and desktop implementations that use a localized default group name resource, this Android implementation hardcodes "Group". The resource tab_settings_group_default_name should be used here for consistency.
| Button("Cancel") { | ||
| dismiss() |
Copilot
AI
Jan 21, 2026
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.
Hardcoded string "Cancel" should be localized. Use NSLocalizedString or Text("cancel_key") for consistency with the rest of the codebase.
| Button("Cancel") { | |
| dismiss() | |
| Button(action: { | |
| dismiss() | |
| }) { | |
| Text("cancel_key") |
| initialItem?.metaData?.title?.let { | ||
| when (it) { | ||
| is TitleType.Text -> it.content | ||
| is TitleType.Localized -> "" // Or resolve string resource if possible |
Copilot
AI
Jan 21, 2026
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.
Comment suggests resolving string resource for TitleType.Localized but the implementation returns an empty string. This could lead to losing the localized title when editing a group with a localized title. Consider implementing the actual resolution or documenting why this is acceptable.
| is TitleType.Localized -> "" // Or resolve string resource if possible | |
| is TitleType.Localized -> stringResource(id = it.resId) |
| initialItem?.metaData?.title?.let { | ||
| when (it) { | ||
| is TitleType.Text -> it.content | ||
| is TitleType.Localized -> "" |
Copilot
AI
Jan 21, 2026
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.
Same issue as in the Android version: TitleType.Localized returns an empty string which could lose the localized title when editing a group. Consider implementing actual resolution or documenting the reasoning.
| is TitleType.Localized -> "" | |
| is TitleType.Localized -> defaultGroupName |
| } | ||
| .padding() | ||
| } | ||
| .navigationTitle("Select Icon") |
Copilot
AI
Jan 21, 2026
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.
Hardcoded string "Select Icon" should be localized. While this string does appear in the Localizable.xcstrings file, it should use NSLocalizedString or similar localization mechanism instead of a hardcoded string for consistency with the rest of the codebase.
| .navigationTitle("Select Icon") | |
| .navigationTitle(NSLocalizedString("Select Icon", comment: "Title for the icon picker screen")) |
| <string name="tab_settings_add_tab">Add Tab</string> | ||
| <string name="tab_settings_edit_group">Edit Group</string> | ||
| <string name="tab_settings_group_empty">No tabs in this group</string> | ||
| <string name="tab_settings_group_name_placeholder">Group name</string> |
Copilot
AI
Jan 21, 2026
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.
Inconsistent string capitalization across platforms. Android uses "Group name" (lowercase 'n') while iOS and desktop use "Group Name" (uppercase 'N'). String resources should be consistent across all platforms for the same UI element.
| <string name="tab_settings_group_name_placeholder">Group name</string> | |
| <string name="tab_settings_group_name_placeholder">Group Name</string> |
fix #1690