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.

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

feat(Android): hide maps #621

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat(Android): hide maps #621

wants to merge 2 commits into from

Conversation

boringcactus
Copy link
Member

Summary

Ticket: 🤖 | Map | Hide maps functionality

Stacked on #618 since the acceptance criteria specify that the location services button is in-scope for this ticket.

iOS

  • If you added any user-facing strings on iOS, are they included in Localizable.xcstrings?
    • Add temporary machine translations, marked "Needs Review"

android

  • All user-facing strings added to strings resource
  • Expensive calculations are run in withContext(Dispatchers.Default) where possible

Testing

Manually validated that everything looks correct with hide maps turned on, except for the stop details page, which looks wrong and needs to be fixed before this PR is actually ready for review. Added a unit test to check that maps are correctly hidden and shown.

Base automatically changed from kb-location-button to main January 6, 2025 15:05
@boringcactus boringcactus force-pushed the mth-android-hide-maps branch from a1c8fb7 to a2b667c Compare January 6, 2025 23:39
@boringcactus boringcactus marked this pull request as ready for review January 6, 2025 23:40
@boringcactus boringcactus requested a review from a team as a code owner January 6, 2025 23:40
@boringcactus boringcactus requested a review from EmmaSimon January 6, 2025 23:40
}
)
LaunchedEffect(nearbyTransit.hideMaps) {
if (nearbyTransit.hideMaps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be !nearbyTransit.hideMaps?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so - if we show maps, the map will be the connection between the LocationDataManager and the ViewportProvider, but if we hide maps, we need to manually wire those together, like we do on iOS.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, okay yes, got it.

}
)
LaunchedEffect(nearbyTransit.hideMaps) {
if (nearbyTransit.hideMaps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, okay yes, got it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants