Emil jiang gamedetails UI #37

EmilJiang · 2025-03-24T01:54:38Z

So, for gamedetails there are still a lot of things to work on. I will attach a picture of what it looks like now, and a list of things to work on. I just wanted to create this PR for comments because there is a lot of changes, and I am not totally done with everything. I tried to follow the code structure of Amy's as much as possible :).

What I did

The networking should be all done, but sometimes it does not load which I think is a backend issue. In the slack, Skye said he will look into it. The Screen with all the components is mostly done.

Specific things that still needs work

hopfully quick ones

Timer for games that have not started yet
Add to calendar
No scoring summary state

Scoring Summary navigation (not sure if helen already added this)

longer ones

Score for the Header (right now i have 0's as placeholders). I do not know how IOS is getting there scores from. A potential idea is summing up the periods from the scoreBreakdown, BUT sometimes that is null, so maybe continuously checking if result is not null signaling the end of match
scoreBreakdown for each sport. There is a pdf for how the breakdown works, and I am thinking just to do a huge when statement of if statement. Right now it is a basic score of last one in the list being final score and the rest being the periods
change scoreSummary to be what IOS has instead of figma. Left is Figma, right is IOS.

Current result

… emil_jiang_gamedetails_ui

zachseidner1

Pass CI before requesting review please. Thank you!

zachseidner1

Emil, this is a really strong first draft, especially as a new member. Really great work. There are still some fixes to be made, but you did a nice job. Also please provide a way for me to test the game details screen! Right now the function is just unused.

app/src/main/java/com/cornellappdev/score/viewmodel/GameDetailsViewModel.kt

app/src/main/java/com/cornellappdev/score/util/GameDataUtil.kt

app/src/main/java/com/cornellappdev/score/util/DateUtil.kt

app/src/main/java/com/cornellappdev/score/model/ScoreRepository.kt

app/src/main/java/com/cornellappdev/score/screen/GameDetailsScreen.kt

zachseidner1

I found some more UI issues: you should update this screen so that the padding matches the Figma page. Please use dev mode to see the padding easier, and add the missing score header.

We can fix the score box card in a separate PR since that's not something you worked on.

EmilJiang · 2025-04-06T04:14:28Z

UI fixes still need to be done, but I think all the logic should be done.

EmilJiang · 2025-04-09T05:07:12Z

I have added all the functionality, and done the UI changes for most. I am still confused on the navigation header padding for the top. In the image below, it cant be 56 because that includes the top of the phone too I think.

Testing is the same as before with a button on the top which sometimes does not work if scraping is done. Some things that still need to be done are navigation and scoring summary UI fix to match with IOS. I will ask for the design tomorrow

zachseidner1

Great job on this Emil! So happy to see this page coming together. I left a few comments for you to address 🫡

app/src/main/java/com/cornellappdev/score/util/DateUtil.kt

app/src/main/java/com/cornellappdev/score/util/GameDataUtil.kt

zachseidner1 · 2025-04-09T12:52:17Z

app/src/main/java/com/cornellappdev/score/util/CalendarUtil.kt

+        putExtra(CalendarContract.EXTRA_EVENT_END_TIME, endMillis)
+    }
+
+    if (intent.resolveActivity(context.packageManager) != null) {


What's the deal with this if statement?

zachseidner1 · 2025-04-09T12:53:08Z

app/src/main/java/com/cornellappdev/score/util/CalendarUtil.kt

+import com.cornellappdev.score.model.DetailsCardData
+import java.time.ZoneId
+
+fun addToCalendar(context: Context, details: DetailsCardData) {


Can we get a function spec, and have you tested this?

Also, this function shouldn't be specific to DetailsGameCard. Maybe instead it could take in some kind of CalendarEvent data class that takes in the minimum data needed to add an event to a calendar?

app/src/main/java/com/cornellappdev/score/components/NavigationHeader.kt

app/src/main/java/com/cornellappdev/score/model/ScoreRepository.kt

zachseidner1 · 2025-04-09T12:59:18Z

app/src/main/java/com/cornellappdev/score/screen/HomeScreen.kt

        modifier = Modifier
            .background(Color.White)
    ) {
+        Button(onClick = { navigateToGameDetails(true) }) { }


This should be of type () -> Unit not (Boolean) -> Unit

zachseidner1

hey sorry for responding to this a bit late, but thank you for pushing those fixes! for the boolean, navigateToGameDetails should not take in a boolean as an argument. It should take in a string instead. Then we should pass in the string to the navController here instead of the empty string.

score-android/app/src/main/java/com/cornellappdev/score/nav/root/RootNavigation.kt

Lines 108 to 110 in b51b133

    
           HomeScreen(navigateToGameDetails = { 
        
               navController.navigate(ScoreRootScreens.GameDetailsPage("")) 
        
           })

Also, do you think you’re able to resolve merge conflicts on your own, or do you want me to help out with that? it seems like there are a lot so lmk what you think.

EmilJiang · 2025-04-14T21:45:02Z

I added navigation to the components to gameDetails. One thing I noticed though was the padding is kind of off. There seems to be hardcoded values, but figma does not show. The "add to calendar" is off the page.

There no numbers attached to spacing between days left and add to calendar. Also, I do not see an example with the bottom navigation. So, I do not know what the padding should be with the bottom navigation or if it should be scrollable

zachseidner1

For situations like these, we can use Spacers with weight modifier. This way it will take up as much space as it can, but it will never cut off the button. We can kinda eye-ball the Figma as a general guideline for weight ratios.

…s_ui # Conflicts: # app/src/main/java/com/cornellappdev/score/components/GamesCarousel.kt # app/src/main/java/com/cornellappdev/score/screen/HomeScreen.kt # app/src/main/java/com/cornellappdev/score/screen/PastGamesScreen.kt

… on game details

zachseidner1

Thank you for responding to my comments! I have a few more 😭 but these are mostly minor. I want to say you did a really great job with this! I really appreciate you being receptive and being able to address most things I say quite quickly.

Make sure to pull before beginning work again since I made a couple small fixes to this branch.

Also, the scoring summary shouldn't be a LazyColumn, it should just only show the first 3 items. You can use Kotlin's Iterable<T>.take extension function for this.

zachseidner1 · 2025-04-15T00:15:51Z

app/src/main/java/com/cornellappdev/score/components/FeaturedGameCard.kt


 @Composable
 fun FeaturedGameCard(
+    id: String,


We should never require an id to show a Composable, the only parameters Composables should take are things related to its view state, or click methods. So really, this composable should have an onClick: () -> Unit method. Then we would write:

onClick = { onClick(game.id) }

And pass it through like that. Lmk if that makes sense.

zachseidner1 · 2025-04-15T00:17:27Z

app/src/main/java/com/cornellappdev/score/screen/GameDetailsScreen.kt

+                        painterResource(R.drawable.ic_calendar),
+                        onClick = {
+                            gameCard.toCalendarEvent()?.let { event ->
+                                addToCalendar(context = context, event)


Optional nit: I don't think you need to pass in context as a named parameter, feels a bit redundant.

The addTocalendar still does not work, so I think I will be working on that as the first issue.

app/src/main/java/com/cornellappdev/score/components/PastGameCard.kt

app/src/main/java/com/cornellappdev/score/model/ScoreRepository.kt

app/src/main/java/com/cornellappdev/score/util/TestingConstants.kt

EmilJiang · 2025-04-15T02:01:48Z

I think in this case, GameCardData needs to have an id because the uistate uses GameCardData. If there is a better way, please let me know!

zachseidner1 · 2025-04-15T02:05:12Z

The addTocalendar still does not work, so I think I will be working on that as the first issue.

Are you sure add to calendar doesn't work? I tried it and it opened google calendar, but not much else happened. Does the same issue happen on your end?

zachseidner1

Finally ready to merge this in! Thank you so much again for responding to all of my comments. I think we've had good discussion and I hope you've learned a lot!

Yes that was my bad about the GameCardData, it's fine for it to have an ID. But it definitely shouldn't be a parameter to the Composable and I'm glad you were able to remove that.

EmilJiang added 6 commits March 13, 2025 11:49

gamedetails networking

826118f

all PR comments addressed except for toResult

8cbd945

fixed data classes to match schema

cd5cf16

Merge branch 'main' of https://github.com/cuappdev/score-android into…

e2b5bce

… emil_jiang_gamedetails_ui

merge

7945993

first draft

002a60c

EmilJiang requested a review from zachseidner1 March 24, 2025 01:54

zachseidner1 requested changes Mar 24, 2025

View reviewed changes

lint fix

e86d68b

zachseidner1 self-requested a review March 24, 2025 14:57

zachseidner1 mentioned this pull request Mar 24, 2025

Scoring summary school name checks #38

Closed

zachseidner1 requested changes Mar 24, 2025

View reviewed changes

nav

f621fd2

zachseidner1 reviewed Mar 24, 2025

View reviewed changes

EmilJiang added 3 commits March 27, 2025 21:34

merge

df67add

pr changes

0a44180

new changes

c25f927

EmilJiang and others added 3 commits April 9, 2025 00:59

ui changes

567d295

minor fix

d57b258

Merge branch 'main' into emil_jiang_gamedetails_ui

e5130a0

todo

c26c0d4

EmilJiang requested a review from zachseidner1 April 9, 2025 05:13

zachseidner1 requested changes Apr 9, 2025

View reviewed changes

pr changes

739c73d

EmilJiang requested a review from zachseidner1 April 14, 2025 04:41

zachseidner1 requested changes Apr 14, 2025

View reviewed changes

EmilJiang added 2 commits April 14, 2025 16:42

merge

dea4ad2

test

c59f6ab

nav fix

2d72680

EmilJiang requested a review from zachseidner1 April 14, 2025 21:45

zachseidner1 reviewed Apr 14, 2025

View reviewed changes

center button and fix padding

8c3264b

EmilJiang requested a review from zachseidner1 April 14, 2025 22:04

zachseidner1 added 3 commits April 14, 2025 19:37

Add preview, fix small score box thing, navigation bar not shown when…

2512179

… on game details

Ellipses

d337068

zachseidner1 reviewed Apr 15, 2025

View reviewed changes

pr

a54bdda

EmilJiang requested a review from zachseidner1 April 15, 2025 02:03

zachseidner1 approved these changes Apr 15, 2025

View reviewed changes

EmilJiang merged commit 63ae483 into main Apr 15, 2025
1 check passed

EmilJiang deleted the emil_jiang_gamedetails_ui branch April 15, 2025 02:10

	HomeScreen(navigateToGameDetails = {
	navController.navigate(ScoreRootScreens.GameDetailsPage(""))
	})

Emil jiang gamedetails UI #37

Emil jiang gamedetails UI #37

Uh oh!

Conversation

EmilJiang commented Mar 24, 2025

Uh oh!

zachseidner1 left a comment

Choose a reason for hiding this comment

Uh oh!

zachseidner1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zachseidner1 left a comment • 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!

EmilJiang commented Apr 6, 2025

Uh oh!

EmilJiang commented Apr 9, 2025

Uh oh!

zachseidner1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zachseidner1 Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

zachseidner1 Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zachseidner1 Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

zachseidner1 left a comment • 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!

EmilJiang commented Apr 14, 2025

Uh oh!

zachseidner1 left a comment

Choose a reason for hiding this comment

Uh oh!

zachseidner1 left a comment

Choose a reason for hiding this comment

Uh oh!

zachseidner1 Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

zachseidner1 Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

EmilJiang Apr 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

EmilJiang commented Apr 15, 2025

Uh oh!

zachseidner1 commented Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zachseidner1 left a comment

Choose a reason for hiding this comment

zachseidner1 left a comment •

edited

Loading

zachseidner1 left a comment •

edited

Loading

zachseidner1 commented Apr 15, 2025 •

edited

Loading