-
Notifications
You must be signed in to change notification settings - Fork 498
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
Delight edit layout experiment #6384
Conversation
- First implementation
# Conflicts: # Riot/Generated/UntranslatedStrings.swift
- Update after reviews
- Added new analytics
- Last tweaks
- fixed theme issues
- fixed build after merge - renamed AllChats classes
- new Build settings newAppLayoutEnaled
- Removed the Pinned space feature
- Removed the All chats layout editor screen - Added feature flag - Added context menus
- Removed analytics used for IA test
📱 Scan the QR code below to install the build for this PR. If you can't scan the QR code you can install the build via this link: https://i.diawi.com/4UFsmf |
- Code cleanup
Codecov Report
@@ Coverage Diff @@
## develop #6384 +/- ##
===========================================
- Coverage 6.18% 6.16% -0.02%
===========================================
Files 1453 1462 +9
Lines 155874 157146 +1272
Branches 62626 63159 +533
===========================================
+ Hits 9636 9689 +53
- Misses 145844 147054 +1210
- Partials 394 403 +9
Continue to review full report at Codecov.
|
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.
Amazing, very nicely separated with the BuildSetting here 👍
I've left comments inline (sorry there's quite a lot 🫣)
One thing I wasn't sure how to summarise inline though was that the use of recent
(s
) for breadcrumbs is a little bit confusing. Especially when you end up with RecentsDataSourceSectionTypeRecentRooms
/RecentsListServiceSectionRecents
where the recents lists has a recents section. Not sure if it would be too big a change at this point to use breadcrumbs
everywhere though 😕.
Riot/Assets/Images.xcassets/Home/home_fab_create_room.imageset/Contents.json
Show resolved
Hide resolved
Co-authored-by: Doug <[email protected]>
- Update after code review
…vector-im/element-ios into gil/6079-Delight_Edit_layout_experiment
- Changed Recents type to Breadcrumbs
- Update after code review
- Update after code review
Riot/Modules/Common/Recents/Service/MatrixSDK/RecentsListService.swift
Outdated
Show resolved
Hide resolved
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.
Looking good to me! A few more small comments inline.
A couple of weird things I notices while testing:
Toolbar in landscape mode | Build flag off, dark theme selected when OS is in light mode (status bar fixed by #6506) |
---|---|
// Need to set `showAllRoomsInHomeSpace` to `true` for the new App Layout | ||
if (BuildSettings.newAppLayoutEnaled) | ||
{ | ||
RiotSettings.shared.showAllRoomsInHomeSpace = YES; | ||
} |
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.
Unrelated to this PR, but I think if this happens, it would be nice to see metaspaces coming to mobile like we have on Web e.g. a "Chats outside of a space" space.
Riot/Modules/Common/Recents/Service/MatrixSDK/RecentsListService.swift
Outdated
Show resolved
Hide resolved
Riot/Modules/Common/Recents/Service/MatrixSDK/RecentsListService.swift
Outdated
Show resolved
Hide resolved
Riot/Modules/Common/Recents/Service/RecentsListServiceProtocol.swift
Outdated
Show resolved
Hide resolved
Riot/Modules/Common/Recents/Service/MatrixSDK/RecentsListService.swift
Outdated
Show resolved
Hide resolved
Everything's fixed now. Nice catch! Thank you :) |
- Update after code review
Kudos, SonarCloud Quality Gate passed! |
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.
Looks good to me. Great job 👏
🎉🎉🎉 |
resolves #6079, resolves #6406, resolves #6407, resolves #6409, resolves #6510, resolves #6509
Scrolling:
Filter:
Edit layout:
Other context menus: