-
Notifications
You must be signed in to change notification settings - Fork 1
[23657] BottomSheet cards refactoring #211
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
base: develop
Are you sure you want to change the base?
Conversation
…figuration and decoupled navigation
… task/23657-bottom-sheet-cards-refactoring
- TripResultPagerFragment.java convertion to kotlin - TKUICardNavigator introduction - update TKUICardViewControllerManager to return committed fragment on showCard - TripResultPagerFragment view pager implementation to use view pager 2 and added new adapter TripGroupsPagerAdapter -
…ap function and update popBackStack to include popBackStackImmediate call - [TripGo] update HomeSearchNavigator to modify calling of SearchCardFragment to use popBackStackImmediate showCard - [TripGo] update LocationChooserFragment to extend TKUICardBaseFragment and implement it's methods and replace event bus calls to use navigator class - [TripGo] update RouteCardFragment to extend TKUICardBaseFragment and implement it's methods - [TripGo] add RouteNavigator for all fragment transaction/navigation related to routing - [TripGo] update SearchCardFragment to extend TKUICardBaseFragment and implement it's methods and replace event bus calls to use navigator class - [TripGo] add SearchNavigator for all fragment transaction/navigation related to search - [TripGo] update TimetableFragment to extend TKUICardBaseFragment and implement it's methods - [TripGo] add TimetableNavigator for all fragment transaction/navigation related to timetable - [TripKitUI] update TKUICardHost to modify popBackstack call and add getMap call - [TripKitUI] update TKUICardNavigator to add handling for configs and parent fragment - [TripKitUI] update TKUICardViewControllerManager to add get fragment by tag and by id functions
…t bus and replaced it with the navigator class - [TripGo] update RouteNavigator to add function to load the route to open TripResultsFragment - [TripGo] update TripResultsFragment to TKUICardBaseFragment and implement its members - [TripKitUI] update TKUICardBaseFragment to add close action handling
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.
Left a few comments, but no changes requested — everything looks good.
sharedViewModel.apply { | ||
observe(pageIndex) { | ||
it?.let { | ||
if (!fromPageListener) { |
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.
Well written code, can improve readability by utilizing return@observe
but not necessary.
|
||
} | ||
|
||
fun setState(state: Int) { |
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.
This could benefit from a safety check similar to the one used in restore()
to guard against invalid states.
} | ||
|
||
fun push() { | ||
behavior.let { |
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.
let
seems to be unused here.
transaction.addToBackStack(fragmentClass.name) | ||
} | ||
|
||
transaction.commitAllowingStateLoss() |
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 result in hard to debug issues.
currentFragment = fragment | ||
|
||
val transaction = fragmentManager.beginTransaction() | ||
.replace(contentFrameId, fragment) |
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.
Adding tag can be of help.
behavior.isHideable = false | ||
|
||
// ✅ Delay fragment commit until container is attached | ||
Handler(Looper.getMainLooper()).postDelayed({ |
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.
Instead of adding delays, might be better to use post {}
.
This pull request is stale because it has been open 5 days with no activity. |
…refactoring # Conflicts: # TripKitAndroidUI/src/main/java/com/skedgo/tripkit/ui/controller/tripdetailsviewcontroller/TKUITripDetailsViewControllerFragment.kt # TripKitAndroidUI/src/main/java/com/skedgo/tripkit/ui/tripresult/TripResultPagerFragment.kt
❌ Unit tests failed! @MichaelReyes, please review and fix the issues in the unit tests. Test results are available under the "Artifacts" section of this run in GitHub Actions. |
❌ Unit tests failed! @MichaelReyes, please review and fix the issues in the unit tests. Test results are available under the "Artifacts" section of this run in GitHub Actions. |
- [TripGo] Fix WLs build issues after merging changes from the latest develop
❌ Unit tests failed! @MichaelReyes, please review and fix the issues in the unit tests. Test results are available under the "Artifacts" section of this run in GitHub Actions. |
✅ Unit tests passed successfully! Test results are available under the "Artifacts" section of this run in GitHub Actions. Please ensure the code is reviewed before proceeding with the merge. |
… need to press close twice to go back - Fix trip alert toggle not working
This pull request is stale because it has been open 5 days with no activity. |
…s when adding to backstack
✅ Unit tests passed successfully! Test results are available under the "Artifacts" section of this run in GitHub Actions. Please ensure the code is reviewed before proceeding with the merge. |
❌ Unit tests failed! @MichaelReyes, please review and fix the issues in the unit tests. Test results are available under the "Artifacts" section of this run in GitHub Actions. |
✅ Unit tests passed successfully! Test results are available under the "Artifacts" section of this run in GitHub Actions. Please ensure the code is reviewed before proceeding with the merge. |
This pull request is stale because it has been open 5 days with no activity. |
✅ Unit tests passed successfully! Test results are available under the "Artifacts" section of this run in GitHub Actions. Please ensure the code is reviewed before proceeding with the merge. |
…om TripKitMapFragment - update HomeNavigator to change poi marker state restoration trigger from TripDetailsFragment to TripResultsFragment - add MapComponent for map related components for injection - update TripDetailsFragment to remove poi markers state update since it's moved to TripResultsFragment - update TripGoSDK to add MapComponent - update TripResultsFragment to initialize and pass mapContributor using TripResultMapContributor to TripResultListFragment - move TripResultMapContributor from TripKitUI to TripGoSDK and use the new TripResultListMapContributor interface from TripKitUI - add MapExtensions.kt to handle all extensions and constants for Map related stuffs - update TripKitMapFragment to add handling of passing markerManager to TripResultListMapContributor, handling for showing/hiding markers and getting/creating marker collections - update TripResultListFragment and update how mapContributor is set and handled - remove old TripResultListMapContributor.kt that's replaced with TripResultListMapContributor interface - add TripResultListMapContributor interface so map contributor for TripResultListFragment can be more configurable - update TripResultMapContributor and allow setting of MarkerManager and only create a new manager if it's null. Also update handling of marker collections
✅ Unit tests passed successfully! Test results are available under the "Artifacts" section of this run in GitHub Actions. Please ensure the code is reviewed before proceeding with the merge. |
This pull request is stale because it has been open 5 days with no activity. |
…refactoring # Conflicts: # tripkit-android
❌ Unit tests failed! @MichaelReyes, please review and fix the issues in the unit tests. Test results are available under the "Artifacts" section of this run in GitHub Actions. |
…ser waiting for location data.
❌ Unit tests failed! @MichaelReyes, please review and fix the issues in the unit tests. Test results are available under the "Artifacts" section of this run in GitHub Actions. |
❌ Unit tests failed! @MichaelReyes, please review and fix the issues in the unit tests. Test results are available under the "Artifacts" section of this run in GitHub Actions. |
✅ Unit tests passed successfully! Test results are available under the "Artifacts" section of this run in GitHub Actions. Please ensure the code is reviewed before proceeding with the merge. |
This pull request is stale because it has been open 5 days with no activity. |
… fallback to proceed without checking location
✅ Unit tests passed successfully! Test results are available under the "Artifacts" section of this run in GitHub Actions. Please ensure the code is reviewed before proceeding with the merge. |
- add Room entities: ScheduledStopEntity, LocationEntity, ScheduledStopDownloadHistoryEntity - add ScheduledStopWithLocation data class for joined queries - add ScheduledStopDao with CRUD operations and bounds queries - add ScheduledStopMapper to convert between Room entities and domain models - add ScheduledStopDatabase as main Room database class - update StopsPersistor to create Room entities directly instead of ContentValues - update ScheduledStopRepository to use Room queries instead of ContentProvider - add simplified bounds queries without download history requirement - update Dagger module to provide Room components - remove cursor-based approach as requested by user
✅ Unit tests passed successfully! Test results are available under the "Artifacts" section of this run in GitHub Actions. Please ensure the code is reviewed before proceeding with the merge. |
…for zoom levels 8.1f-12.9f - Updated zoom level ranges: REGION (8.1f-12.9f), HYBRID (13.0f-15.1f), LOCAL (≥15.2f) - Implemented hybrid approach where 13.0f-15.1f loads both region and local level stops - Ensured ViewPort.CloseEnough is created for all valid zoom levels to trigger API calls - Fixed issue where REGION level markers weren't showing due to broken zoom level detection
❌ Unit tests failed! @MichaelReyes, please review and fix the issues in the unit tests. Test results are available under the "Artifacts" section of this run in GitHub Actions. |
- add singleton class MapData and cache the regional stop marker options - update MapViewModel to modify viewPort distinctUntilChanged and include zoom level checking so it'll still trigger even on the same region on a region level zoom - local storage updates for schedule stops for Room db
✅ Unit tests passed successfully! Test results are available under the "Artifacts" section of this run in GitHub Actions. Please ensure the code is reviewed before proceeding with the merge. |
✅ Unit tests passed successfully! Test results are available under the "Artifacts" section of this run in GitHub Actions. Please ensure the code is reviewed before proceeding with the merge. |
Pull Request (PR) Checklist
Thank you for your contribution! Please confirm that you've checked all the boxes below before submitting your PR. Use
[x]
to check a box, e.g.,[x]
, and make sure there's no space around the brackets.PR Context
Changes
Describe your changes in detail, highlighting the problem it solves or the feature it adds.
Retained Bottom Sheet Implementation
FrameLayout
+BottomSheetBehavior
insideHomeFragment
to support touch interactions with the map behind.Introduced Modular Architecture Components
TKUICardHost
Interface contract to host fragments inside the bottom sheet.
TKUICardNavigator
Per-screen navigator classes (e.g.,
SearchNavigator
,RouteNavigator
,TripNavigator
) that handle navigation logic per card.TKUICardViewControllerManager
Utility class that handles fragment transactions and setup logic dynamically through the
FragmentManager
.Refactored to Use
TKUICardBaseFragment
Each bottom sheet screen now extends
TKUICardBaseFragment
and defines its own:behaviorState
peekHeightResourceValue
isHideable
This makes each screen self-contained and allows custom behavior configurations.
Checklist for Reviewers
Documentation and Code Quality
Testing and Reliability
Error Handling and Logging
Testing Procedure
If applicable, provide steps or commands for testing your changes. This can help reviewers and testers.
Work-in-Progress (WIP)
List any remaining work or areas that need additional focus. This section can be updated as the work progresses.
Remember to keep this template updated based on the feedback and evolving project standards.