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

Feature/sort by distance #382

Merged
merged 4 commits into from
Jun 2, 2024
Merged

Feature/sort by distance #382

merged 4 commits into from
Jun 2, 2024

Conversation

alejandrocalles
Copy link
Contributor

@alejandrocalles alejandrocalles commented Jun 2, 2024

Sort events by distance.

Display distance to user in EventListItem, but only on the HomeScreen.

Closes #383 and #384.

@alejandrocalles alejandrocalles added the enhancement New feature or request label Jun 2, 2024
@alejandrocalles alejandrocalles added this to the Milestone 4 milestone Jun 2, 2024
@alejandrocalles alejandrocalles self-assigned this Jun 2, 2024
@alejandrocalles alejandrocalles force-pushed the feature/sort-by-distance branch 2 times, most recently from ffcaa69 to 1b51684 Compare June 2, 2024 14:30
@srsingh04 srsingh04 self-requested a review June 2, 2024 15:17
@alejandrocalles
Copy link
Contributor Author

alejandrocalles commented Jun 2, 2024

User should be able to sort events by distance ascending or descending. By default, events are sorted by date, so the distance sort is applied on top, but this doesn't really matter as it is unlikely to have two events at the same coordinates.

srsingh04
srsingh04 previously approved these changes Jun 2, 2024
Copy link
Contributor

@srsingh04 srsingh04 left a comment

Choose a reason for hiding this comment

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

LGTM! Left some minor comments, but can be merged into the main regardless due to time constraints.

@Provides
fun provideGPSService(application: Application): GPSService {
val context = application.applicationContext
return GPSServiceImpl(context)
Copy link
Contributor

Choose a reason for hiding this comment

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

Knitpick/Suggestion for next time: Maybe instead of defining context as a variable, use its declaration directly in the return statement since we only use it once?

val fmt = { format: String, value: Double ->
String.format(Locale.getDefault(), format, value)
}
if (it < 1000) fmt("%.0fm", it) else fmt("%.1fkm", it / 1000.0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Knitpick: good practise to not Magic values in the code: 1000.0 can be extracted into a variable?

canModifyEvent = canModifyEvent,
distance = userLocation?.let { event.location.toLatLng().distanceTo(it) },
)
Spacer(modifier = Modifier.height(12.dp))
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly for the height value, could be extracted into a variable to b e able to make modifications easily by someone else in the future

@unglazedstamp unglazedstamp requested review from unglazedstamp and removed request for unglazedstamp June 2, 2024 16:20
@alejandrocalles
Copy link
Contributor Author

Those are good points! I have addressed them.

@alejandrocalles alejandrocalles enabled auto-merge (rebase) June 2, 2024 16:30
Copy link

@unglazedstamp unglazedstamp left a comment

Choose a reason for hiding this comment

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

The changes requested by @srsingh04 have been made

Copy link

sonarcloud bot commented Jun 2, 2024

@alejandrocalles alejandrocalles merged commit 2c5bff7 into main Jun 2, 2024
2 checks passed
@alejandrocalles alejandrocalles deleted the feature/sort-by-distance branch June 2, 2024 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done in sprint 10
Development

Successfully merging this pull request may close these issues.

Display event distance to user
3 participants