-
Notifications
You must be signed in to change notification settings - Fork 2
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/event listing - Implement Event List Overview with Mock Data, UI Enhancements, and Unit Tests #29
Conversation
- Create `Event.kt` data class to represent event entities. - Define `EventRepository` interface for event-related data operations. - Implement `EventRepositoryFirestore` to fetch events from Firebase Firestore. Theses implementations are very dumb, as someone else will take care of the Events database. I made it in order to start doing the events list
- Implemented EventRepositoryMock to provide mock data since EventRepository is not made yet - Mock data includes sample events for testing purposes
- Created EventListViewModel to manage event data using StateFlow. - Added a function to load mock events for testing and development. - Provided ViewModelFactory for proper lifecycle management.
… animation and dynamic layout - Implemented nice event layout with gradient colors. - Implemented a smooth sliding animation for tab selection between "All" and "Following". - Added dynamic width calculation for tab text to ensure proper layout. - Introduced tracking of X coordinates for both tabs to calculate the selected tab's position and width. - Converted pixel sizes to density-independent pixels (dp) for better scalability across devices. - Enhanced event list handling to display a message when no events are available, improving user experience.
… animation and dynamic layout - Implemented nice event layout with gradient colors. - Implemented a smooth sliding animation for tab selection between "All" and "Following". - Added dynamic width calculation for tab text to ensure proper layout. - Introduced tracking of X coordinates for both tabs to calculate the selected tab's position and width. - Converted pixel sizes to density-independent pixels (dp) for better scalability across devices. - Enhanced event list handling to display a message when no events are available, improving user experience.
- Created a data class `EventTypeColor` to represent event types and their associated colors. - Implemented a function `getColorForEventType` to retrieve the color based on the event type.
…es to Event data class
- Added a preview composable function `EventListOverviewPreview` for `EventListOverview`. - Enhanced the `EventItem` UI to include a background image and improved text styling. - Added utility functions for color manipulation and contrast calculation. - Updated imports and dependencies to support new features.
- Add tests for tab switching and animations in the EventListOverview component. - Add a test for displaying the empty event list message when no events are available. - Add a test for rendering a non-empty event list and verifying the titles and descriptions of the events. - Add a test to verify the functionality of the "Map" button.
- Added test tags for various UI components in the `EventListOverview` and `EventItem` composables to facilitate automated testing.
-will remake one later
There is a lot of duplicated code, but it's only for the EventListRepositoryMock for test reasons so it will be destroyed when I will delete my mock |
The image in "model/events/events_picture" is already in "resources". The "event_picture" package should not exist in "model/events". However, it can be a sub package of "resources" |
val title: String, | ||
val catchy_description: String, | ||
val description: String, | ||
val date: String, // Consider using Date type with proper formatting |
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.
Is there a good reason to keep the "date" attribute as a String for now ? Why isn't this already a "Date" type ?
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.
Indeed, we should use the Firebase Timestamp
class.
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.
Indeed, we should use the Firebase
Timestamp
class.
Agreed.
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 class was only a 'mock' class to wait for the real Event Repository structure
When it will be pushed to main, i will delete my event repository structure and connect my code with the one in main
val catchy_description: String, | ||
val description: String, | ||
val date: String, // Consider using Date type with proper formatting | ||
val location: String, |
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.
The "location" attribute should be a "Location" type, like used in the bootcamp, or another type, from a new class, to fit better with EPFL geography.
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.
The "location" attribute should be a "Location" type, like used in the bootcamp, or another type, from a new class, to fit better with EPFL geography.
Yes. We do have a Location data class available which can and should be used
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.
Agreed, I believe Firebase provides a GeoPoint data class we could use.
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.
same as before, i just did it blindly but i will destroy this class for the better one
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.
Agreed, I believe Firebase provides a GeoPoint data class we could use.
True, however this would only store the coordinates correct?
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.
You can transform Coordinates into a place with Google Maps API
I think the best thing to do would be to ask the Event creator precise coordinates (as our map feature is based on precision of the event) and then we could show himthe output of Maps API for example 'Plage de Vidy'
Then he can agree to it or edit it, and the location as a text would be what people see on the event page, but if they clic on the map the pin point location would be the precise coordinates of the event
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.
Yes correct I just mean we will still need to hava a String
field for the name of the location, so we either keep Location.kt
as is, or use the GeoPoint
data class instead of having two Double
fields
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.
True, however this would only store the coordinates correct?
Yes, the GeoPoint
class contains only a longitude and latitude property, but its advantage is the automatic serialization and hydration when sending/retrieving from Firebase. Maybe the correct way to go is a wrapper class that contains a GeoPoint
in addition to some information about the event's location.
val description: String, | ||
val date: String, // Consider using Date type with proper formatting | ||
val location: String, | ||
val main_type: String, |
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.
The "main_type" attribute should probably be an Enumeration of type of event. This will also be useful in "EventTypeColor"
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.
Yes, kind of like the enum class AssociationType I wrote in the MockAssociations.kt file (which we will also incorporate to the real association data class)
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.
same haha but thank you for the idea
val location: String, | ||
val main_type: String, | ||
val picture: String | ||
) |
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.
Consider also using camelCase over snake_case for attribute, to keep the same naming convention over the whole project
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.
Thanks ! I'm gonna check on the real one to make sure it's in camelCase
import androidx.compose.ui.graphics.Color | ||
|
||
// class for event types and their corresponding colors | ||
data class EventTypeColor(val type: String, val color: Color) |
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.
As said in "Event.kt", the "type" attribute should be an Enumeration of type
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.
Additionally, this class should simply be called EventType
, as its main purpose isn't providing colors, but instead distinguishing event types.
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.
Okay, I will change that ! Thanks
listOf( | ||
EventTypeColor("festival", Color(0xFF6200EE)), // purple | ||
EventTypeColor("apéro", Color(0xFF03DAC5)), // teal | ||
EventTypeColor("soirée", Color(0xFFFF5722)), // deep orange |
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.
Consider translate "type" in english, for consistency (like "apéro" or "soirée")
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.
Thanks !
class EventRepositoryFirestore : EventRepository { | ||
private val firestore = FirebaseFirestore.getInstance() | ||
|
||
// Change the return type to accept a callback |
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.
ChatGPT ?
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 one will be destroyed, as the other for the same explanation
|
||
/** Loads the list of events from the repository and updates the internal [MutableStateFlow]. */ | ||
fun loadEvents() { | ||
_events.value = repository.getEvents() |
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.
When at a later time the EventRepositoryFirestore
is integrated, this will not work, as repository.getEvents
is asynchronous. Consider changing this to use a callback function.
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.
Yes thank you ! Copilot proposed it to me, but as I didn't know how EventRepositoryFirestore would be made, i didn't do it
If that is okay for all of my reviewers, I will change EnventListViewModel.kt to use a callback function, restructure the EventType.kt file and add some consistency in the names. For the rest, as it was only a blind implementation of EventRepository structure in order to wait for the real one (which is now pushed to main), I will just push to the main and then destroy my structure and connect the real one to the rest of my code. Thanks ! |
@Aurelien9Code I believe you can pull the changes from main directly to this branch, without having to merge beforehand. See this. |
thanks ! I'll check this tomorrow morning |
…m for better understanding and utilization
- Updated EventListViewModel to load events asynchronously using coroutines. - Refactored `loadEvents()` to use `viewModelScope.launch` for non-blocking execution.
… (also for test file)
|
||
// these twos functions might be useful later | ||
|
||
/*fun getContrastingColorBlackAndWhite(backgroundColor: Color): Color { |
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.
Consider removing these comments in a later pull request.
All the changements required are now done ! Thanks ! |
LGTM |
Astronomically low code coverage, but apart from that it looks good to me. |
71% will be improved soon for sure ! But it's not that bad... |
Quality Gate failedFailed conditions |
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.
PR Review
-
Improving Description for Clarity: Consider adding titles and subtitles in Markdown to improve readability and make the description clearer. This structure helps reviewers quickly understand what has been implemented and its purpose.
-
Hardcoded Strings: You still have hardcoded strings in your code. It’s better to move them to string resources (strings.xml) to make the code more maintainable. This will also make it easier if you plan to support multiple languages in the future.
-
Comment Removal: It seems there are still comments in your code, such as "these two functions might be useful later", which have not been removed. If you choose to leave comments like these, be more specific about their purpose. Explain why they may be useful later and provide context for future contributors.
-
Inlined Comments: Small suggestion regarding the inlined comments—there are quite a few that explain what the code is doing, which is already clear from the code itself. This can make the code less readable and weigh it down. To keep things clean and easy to follow, we could reduce some of these comments and instead focus on explaining why certain decisions were made or clarifying any complex logic. This will help make the code more concise and maintainable.
-
Positive Feedback: Excellent commit messages and a very well-organized description! Your reviewers provided valuable feedback, and you did a fantastic job implementing their suggestions. Your code is clean, maintainable, and well-documented. Keep up the great work!
style = | ||
TextStyle( | ||
fontSize = | ||
24.sp // Set the font size to 24 sp (scale-independent pixels) |
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 comment is not necessary
|
||
// Clickable text for "Following" | ||
Text( | ||
text = "Following", |
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.
It's a good habit to develop to avoid writing hard coded string in the code, and favor the use of String.xml resources. This will makes it easier to later localize the application. It also makes the app more maintainable as it is easy to modify one text, without having to remember to modify every places that are displaying the hard coded text.
This pull request introduces the foundational structure for the "Events" feature, including a mock implementation of event data handling, UI enhancements, and essential unit tests. The focus is to provide a functional event list view for testing and development purposes until the final event data source is implemented.
Key Features ->
Event Data Handling:
-Created Event.kt data class to represent event entities.
-Implemented EventRepositoryMock for testing, providing sample events as the real event repository is yet to be finalized.
ViewModel and State Management:
-Introduced EventListViewModel with StateFlow to manage event data.
-Mock data is loaded into the ViewModel for use during testing and development.
UI Enhancements:
-Developed a responsive Event List Overview with a tab selection animation (switching between "All" and "Following").
-Created a dynamic layout that adjusts the tab size based on text width and tracks the X coordinates for smooth transitions.
-Designed EventItem UI with gradient colors, background images, and dynamic styling for better user experience.
-Added handling for empty event lists, showing a user-friendly message when no events are available.
-Implemented utility functions for event type color mapping and contrast adjustments.
-Introduced new properties to Event (like catchy_description, main_type, and picture) to enrich the event details.
UI Testing and Preview:
-Added a composable function, EventListOverviewPreview, for easier UI previews in development.
-Enhanced UI components with test tags to support automated UI testing.
Unit Testing:
-Added comprehensive tests for the Event List Overview component, covering tab switching, empty list handling, and rendering of event items.
-Verified UI animations and behavior, including the functionality of the "Map" button.
Let me know if you'd like any modification ! ; )