Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: Integrate folder picker on iPad - WPB-11895 #2156

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

feat: Integrate folder picker on iPad - WPB-11895 #2156

wants to merge 8 commits into from
+136 −51

Conversation

samwyndham
Copy link
Contributor

Issue

This PR integrates the folder picker feature on iPad ONLY hopefully meeting the acceptance requirements outline in https://wearezeta.atlassian.net/browse/WPB-11754.

folders-ipad.mov

Testing

Note: Currently there is no way to add/remove folders on iOS. Instead use the web interface to add/remove folders as needed.

Please test on iPad.

Folders use case

  1. Ensures your account has some folders in it.
  2. Rotate to portrait
  3. Tap the side bar button if the sidebar isn't already showing
  4. Tap Folders from the sidebar.
  5. Verify a popover view opens with a list of folders. No folder is selected.
  6. Tap a folder.
  7. Verify the popover dismisses and the selected folders contents are displayed.
  8. Tap Folders from the sidebar.
  9. Verify a popover view opens with a list of folders. The previously selected folder is still selected.
  10. Tap All from the sidebar.
  11. Verify all conversations are displayed.
  12. Tap Folders from the sidebar.
  13. Verify a popover view opens with a list of folders. No folder is selected.
  14. Rotate to landscape and repeat steps 1 - 12

No folders use case

  1. Ensure your account has no folders.
  2. Tap the side bar button if the sidebar isn't already showing
  3. Tap Folders from the sidebar.
  4. Verify a popover view opens showing an empty state message.
  5. Tap How to add a conversation to a folder.
  6. App navigates to https://support.wire.com/hc/en-us/articles/360002855817-Add-a-conversation-to-a-custom-folder in Safari.
  7. Return to the app.
  8. Close the popover

Checklist

  • Title contains a reference JIRA issue number like [WPB-XXX].
  • Description is filled and free of optional paragraphs.
  • Adds/updates automated tests.

UI accessibility checklist

If your PR includes UI changes, please utilize this checklist:

  • Make sure you use the API for UI elements that support large fonts.
  • All colors are taken from WireDesign.ColorTheme or constructed using WireDesign.BaseColorPalette.
  • New UI elements have Accessibility strings for VoiceOver.

@echoes-hq echoes-hq bot added the echoes: product-roadmap Work aligned with the customer-announced roadmap, targeting a specific release date. label Nov 14, 2024
@samwyndham samwyndham requested review from agisilaos and caldrian and removed request for agisilaos November 14, 2024 08:32
@samwyndham samwyndham changed the base branch from develop to feat/folder-picker-filter-menu-option-WPB-11894 November 14, 2024 12:50
@samwyndham samwyndham mentioned this pull request Nov 14, 2024
Merged
6 tasks
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 14, 2024 •
edited
Loading

Test Results

    2 files    309 suites   3m 1s ⏱️
1 870 tests 1 868 ✅ 0 💤  2 ❌
1 882 runs  1 868 ✅ 0 💤 14 ❌

For more details on these failures, see this check.

Results for commit eba8991.

♻️ This comment has been updated with latest results.

Base automatically changed from feat/folder-picker-filter-menu-option-WPB-11894 to develop November 15, 2024 08:17
@samwyndham samwyndham force-pushed the feat/folder-picker-ipad-WPB-11895 branch from e42293c to 014bbda Compare November 15, 2024 09:04
caldrian
caldrian reviewed Nov 15, 2024
View reviewed changes
WireUI/Sources/WireMainNavigationUI/Protocols/Sidebar/MainSidebarMenuItemRepresentable.swift Show resolved Hide resolved
WireUI/Sources/WireSidebarUI/Views/SidebarViewControllerDelegate.swift
@MainActor
public protocol SidebarViewControllerDelegate: AnyObject {
func sidebarViewControllerDidSelectAccountImage(_ viewController: SidebarViewController)
func sidebarViewController(_ viewController: SidebarViewController, didTapFoldersAt rect: CGRect)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didtapFoldersAt rect sounds really fragile to me.
Is there no other way to know which folder was selected?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@caldrian Maybe this is badly named. This is not about identifying which folder was tapped. It is the frame of the Folders button in the sidebar.

@samwyndham
Merge branch 'develop' into feat/folder-picker-ipad-WPB-11895
fd73e30 
# Conflicts:
#	WireUI/Sources/WireSidebarUI/Views/Preview/SidebarMenuItemPreview.swift
#	WireUI/Sources/WireSidebarUI/Views/SidebarMenuItemView.swift
#	WireUI/Sources/WireSidebarUI/Views/SidebarView.swift
caldrian
caldrian reviewed Nov 15, 2024
View reviewed changes
WireUI/Sources/WireSidebarUI/Views/SidebarMenuItemView.swift Outdated
@@ -87,6 +90,11 @@ struct SidebarMenuItemView<TitleView: View>: View {
.cornerRadius(backgroundCornerRadius)
}
.dynamicTypeSize(...DynamicTypeSize.accessibility1)
.onGeometryChange(for: CGRect.self) { proxy in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what exactly happens here, but could it be that this causes performance overhead when scrolling?
I don't think every menu time view should keep track of its frame on the screen.
Maybe we can brainstorm for a bitter idea?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caldrian caldrian caldrian approved these changes

@agisilaos agisilaos Awaiting requested review from agisilaos

At least 2 approving reviews are required to merge this pull request.

Assignees

@samwyndham samwyndham

Labels
echoes: product-roadmap Work aligned with the customer-announced roadmap, targeting a specific release date.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@samwyndham @caldrian