-
Notifications
You must be signed in to change notification settings - Fork 117
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
Pinned Events Timeline actions and differentiation #3182
Conversation
Generated by 🚫 Danger Swift against d32babd |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3182 +/- ##
===========================================
- Coverage 77.68% 77.48% -0.20%
===========================================
Files 718 719 +1
Lines 56695 57032 +337
===========================================
+ Hits 44041 44193 +152
- Misses 12654 12839 +185
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
48c5398
to
d0f1db7
Compare
d0f1db7
to
97af468
Compare
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.
Veery nice 🤩
A thought whilst I was reading through this. I think the notion of isLive
on a timeline is likely wrong now - we should probably be using an enum of case live, detached, pinned
instead. I have a feeling this would mean we didn't need to inject a bunch of those shouldHideStart
/isPinnedEventsTimeline
parameters as they could read this from the TimelineProxy
, TimelineProvider
, TimelineController
or TimelineTableViewController
. Definitely not to fix in this PR though.
Also a couple of thoughts on naming I wasn't sure where to put:
- I don't really think changing
focussedEvent
→focusEvent
is a good change. It doesn't read as clearly aspresentRoom(focussedEvent: theEvent)
to me. - I find it slightly confusing that sometimes it's we say the
pinEvent
and other times we say thepinnedEvent
. I would have thought we always refer to it as the latter and we only use pin event as a verb?
ElementX.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
Outdated
Show resolved
Hide resolved
ElementX/Sources/Screens/RoomScreen/RoomScreenCoordinator.swift
Outdated
Show resolved
Hide resolved
ElementX/Sources/Screens/RoomScreen/RoomScreenCoordinator.swift
Outdated
Show resolved
Hide resolved
ElementX/Sources/FlowCoordinators/PinnedEventsTimelineFlowCoordinator.swift
Outdated
Show resolved
Hide resolved
ElementX/Sources/FlowCoordinators/PinnedEventsTimelineFlowCoordinator.swift
Show resolved
Hide resolved
added also a view in timeline action, that only appears in the pinned events timeline, and a way to ungroup events when the timeline is a pinned events one.
and allow interaction with the map
and updated tests
d816cd9
to
115c0af
Compare
Quality Gate failedFailed conditions |
fixes #3060
Simulator.Screen.Recording.-.iPhone.15.-.2024-08-21.at.13.39.08.mp4