Feature: Implement the event details UI according to the figma. #111

armouldr · 2024-10-29T13:51:40Z

Implemented the UI of Event Details screen according to the Figma. Note that it is not linked to Firebase as this will be a separate task.

Also added EventDetailsTest to test said screen's UI and wrote a method to allow us to get an event by its id, as this will be useful when linking the screen to the backend

Added a savedEvents parameter to keep track of which events the user follows.

Fixed the errors in text files due to the previous refactoring

Added all the composables required by the figma design.

Improved the event details UI to adhere to the figma.

Wrote some UI tests for the Event Screen

Added a test for findEventById method.

oskar-codes

This pull request is an overall good addition to the project, well done!

oskar-codes · 2024-11-03T16:43:24Z

app/src/test/java/com/android/unio/model/firestore/HydrationAndSerializationTest.kt

 class HydrationAndSerializationTest {
-  private val user =
+
+  val user =


Consider leaving these values private.

oskar-codes · 2024-11-03T16:47:45Z

app/src/main/java/com/android/unio/ui/event/EventDetails.kt

+fun EventScreen(
+    navigationAction: NavigationAction,
+    eventId: String,
+    eventListViewModel: EventListViewModel,


Here you should only use a single EventViewModel instead of both the eventListViewModel and eventId. This will make the function declaration more succinct. The instanciation of this EventViewModel could then be placed in MainActivity right before passing it as a parameter to the EventDetails.

By the way, the EventViewModel doesn't exist on your branch, you should pull it from main.

Oh the EventViewModel class didn't exist when I frist wrote my code but that's indeed better suited.

oskar-codes · 2024-11-03T16:51:24Z

app/src/main/java/com/android/unio/ui/event/EventDetails.kt

+private var scope: CoroutineScope? = null
+
+@OptIn(ExperimentalMaterial3Api::class)
+@SuppressLint("UnusedMaterial3ScaffoldPaddingParameter")


You should remove this UnusedMaterial3ScaffoldPaddingParameter suppression, and actually use the padding parameter provided by the Scaffold. This will ensure the content is well within the bounds of the scaffold.

oskar-codes · 2024-11-03T16:51:47Z

app/src/main/java/com/android/unio/ui/event/EventDetails.kt

+      content = { padding ->
+        // Mock image before linking to backend
+        Column(
+            modifier = Modifier.testTag("eventDetailsPage").verticalScroll(rememberScrollState())) {


As mentionned in the previous comment, simply add the .padding(padding) modifier here.

You are right I added the modifier.

oskar-codes · 2024-11-03T16:57:07Z

app/src/main/java/com/android/unio/ui/event/EventDetails.kt

+              Column(
+                  modifier =
+                      Modifier.testTag("eventDetailsInformationCard")
+                          .background(primaryLight)


⚠️ Avoid using explicit theme-independent colors for text and background. This will set the color to a constant value that stays even when changing theme, which removes the purpose of having a theme alltogether. Use instead the associated value: MaterialTheme.colorScheme.primary.

Make sure to also add the appropriate changes to lines 182, 211, 219, 223, 231, 251, and 272. Also for some of these, do you need to specify the color ? Often the default color is good enough.

I hadn't understood how our theme worked ! Should be good now thank you

oskar-codes · 2024-11-03T17:09:09Z

app/src/test/java/com/android/unio/model/event/EventViewModelTest.kt

I believe this file existed before this pull request, but we should consider renaming it to EventListViewModelTest, to avoid confusion with the EventViewModel because this file tests an EventListViewModel, not an EventViewModel.

oskar-codes · 2024-11-03T17:10:33Z

app/src/test/java/com/android/unio/model/event/EventViewModelTest.kt

  }
+
+  @Test
+  fun testFindAssociationById() {


I believe the name of this test has nothing to do with its contents, it might be better to rename it to something else.

oskar-codes · 2024-11-03T17:17:37Z

app/src/main/java/com/android/unio/model/event/EventListViewModel.kt

+   * @param id The ID of the event to find.
+   * @return The event with the given ID, or null if no such event exists.
+   */
+  fun findEventById(id: String): Event? {


This is a good function to have, but as mentionned in a previous comment, consider using the EventViewModel for your pull request. It should contains a single Event, just like the UserViewModel, and should be instanciated when you navigate to an event's details page.
Note that the current EventViewModel is only partially implemented as it is unused, and will require changes before you can use it. Maybe that's something you can do when you link the event details to the backend.

oskar-codes · 2024-11-03T17:20:28Z

app/src/androidTest/java/com/android/unio/ui/event/EventDetailsTest.kt

+class EventDetailsTest {
+  private lateinit var navHostController: NavHostController
+  private lateinit var navigationAction: NavigationAction
+  @Mock private lateinit var collectionReference: CollectionReference


It appears that several of these mocked variables such as db or collectionReference are not used in this test. Consider removing them if they are not necessary.

oskar-codes · 2024-11-03T17:21:14Z

app/src/androidTest/java/com/android/unio/ui/event/EventDetailsTest.kt

+    assertDisplayComponentInScroll(composeTestRule.onNodeWithTag("signUpButton"))
+  }
+
+  private fun assertDisplayComponentInScroll(compose: SemanticsNodeInteraction) {


Great use of helper functions 🚀

Change the way I define the element's color so that they support changing themes.

Fix EventCardTest that couldn't find some elements because the elements are in merged tree

oskar-codes

Well done addressing the changes I requested, this will prove itself considerably useful for the application.

oskar-codes · 2024-11-07T09:41:51Z

app/src/main/java/com/android/unio/MainActivity.kt

+        uid?.let {
+          EventScreen(
+              navigationAction = navigationActions,
+              eventViewModel = EventViewModel(eventRepository, userRepository),


You should extract the instanciation of the EventViewModel at the top of the composable, and wrap it in a remember { ... }

In MainActivity, moved EventViewModel out of the composable. In Serialization, simplified a statement

sonarqubecloud · 2024-11-07T10:03:26Z

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
72.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

feat(model/event): implement getEventById

a5b7f1a

armouldr linked an issue Oct 29, 2024 that may be closed by this pull request

Implement event screen #107

Closed

armouldr added 9 commits October 29, 2024 16:13

feat(model/user): Refactor User data class

98c8eba

Added a savedEvents parameter to keep track of which events the user follows.

chore: Fix test errors

fe2937e

Fixed the errors in text files due to the previous refactoring

Merge branch 'main' into feat/event-details-ui

b313fa2

feat(ui/event):Add all eventDetails UI elements

5772059

Added all the composables required by the figma design.

feat(ui/event): perfect event details UI

21a5ba8

Improved the event details UI to adhere to the figma.

test(ui/event): simple UI tests for event details

db1c727

Wrote some UI tests for the Event Screen

test(model/event): Add test in EventViewModel

8a90eea

Added a test for findEventById method.

Merge branch 'main' into feat/event-details-ui

27fd4dc

chore: ktfmtFormat

28df626

armouldr marked this pull request as ready for review November 1, 2024 00:43

armouldr added 2 commits November 1, 2024 01:51

chore: Correct a test that failed

43cf06e

chore: fix ScreenDisplayingTest

7b1d015

armouldr requested review from AlexeiThornber, Aurelien9Code, Redd87, Romainhir, Zafouche and oskar-codes November 3, 2024 00:30

oskar-codes reviewed Nov 3, 2024

View reviewed changes

armouldr added 9 commits November 5, 2024 16:20

Merge branch 'main' into feat/event-details-ui

eb365b8

chore: merge with main

9284d08

chore:fix errors due to merging with main

77c8522

chore: change color specification

7b15b97

Change the way I define the element's color so that they support changing themes.

chore: fix and optimise tests

132307d

chore: run code cleanup and ktfmt

3390fd0

chore: ktfmtFormat

91b414d

Merge branch 'main' into feat/event-details-ui

44a8c17

chore: ktfmtFormat

304eed4

armouldr added 3 commits November 6, 2024 00:05

chore: fix EventCardTest

75288b5

Fix EventCardTest that couldn't find some elements because the elements are in merged tree

chore: fix EventDetailsTest to pass CI

cd17b46

chore: fix EventDetailsTest to pass CI

969fb6a

armouldr requested a review from oskar-codes November 5, 2024 23:47

oskar-codes approved these changes Nov 7, 2024

View reviewed changes

chore: minor fixes

e1edf8e

In MainActivity, moved EventViewModel out of the composable. In Serialization, simplified a statement

armouldr merged commit 4a0609c into main Nov 7, 2024
2 checks passed

armouldr deleted the feat/event-details-ui branch November 7, 2024 10:04

Feature: Implement the event details UI according to the figma. #111

Feature: Implement the event details UI according to the figma. #111

Uh oh!

Conversation

armouldr commented Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oskar-codes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oskar-codes Nov 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oskar-codes Nov 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oskar-codes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Nov 7, 2024

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

armouldr commented Oct 29, 2024 •

edited

Loading

oskar-codes Nov 3, 2024 •

edited

Loading

oskar-codes Nov 3, 2024 •

edited

Loading