-
Notifications
You must be signed in to change notification settings - Fork 0
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
Integrated backend and fixed UI #4
base: main
Are you sure you want to change the base?
Conversation
// private(set) lazy var apollo: ApolloClient = { | ||
// let url = URL(string: "https://yourgraphqlendpoint.com/graphql")! | ||
// return ApolloClient(url: url) | ||
//}() |
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.
Remove commented out code if not necessary.
// for datum in gamesData { | ||
// print("Game of \(datum.sport) and \(datum.gender)") | ||
// } |
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.
Remove commented out code if not necessary.
(gender == nil || game.gender == gender) && | ||
(sport == nil || game.sport == sport) | ||
} | ||
// print("game is empty: " + String(filteredGames.isEmpty)) |
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.
Remove commented out code if not necessary.
func ordinalSuffix(for number: Int) -> String { | ||
let lastDigit = number % 10 | ||
let lastTwoDigits = number % 100 | ||
|
||
if lastTwoDigits >= 11 && lastTwoDigits <= 13 { | ||
return "th" | ||
} | ||
|
||
switch lastDigit { | ||
case 1: return "st" | ||
case 2: return "nd" | ||
case 3: return "rd" | ||
default: return "th" | ||
} | ||
} | ||
|
||
func ordinalNumberString(for number: Int) -> String { | ||
return "\(number)\(ordinalSuffix(for: number))" | ||
} |
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.
Could you put these functions inside of a class called OrdinalSuffix
?
// @Published var allGames: [Game] = [] | ||
// @Published var upcomingGames: [Game] = [] | ||
// @Published var pastGames: [Game] = [] | ||
|
||
init() { | ||
// fetchAllGames { [weak self] game in | ||
// DispatchQueue.main.async { | ||
// self?.allGames = games | ||
// } | ||
// } | ||
} |
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.
Remove if unused.
let now = Date() | ||
let twoHours: TimeInterval = 2 * 60 * 60 | ||
let calendar = Calendar.current | ||
let startOfToday = calendar.startOfDay(for: now) |
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.
I would combine these onto the startOfToday
line, except for twoHours
. If it makes more sense, you could also use Date.now for the current date.
let now = Date() | ||
let calendar = Calendar.current | ||
let startOfToday = calendar.startOfDay(for: now) |
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 above comment.
// } | ||
} | ||
|
||
private func getUpcomingGames(allGames: [Game]) -> [Game] { |
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 might be able to make this code more concise by using the filter and sort function on the allGames
array. I believe you can filter the games today, then sort by their dates (assuming that's what your array looks like).
return upcomingGames | ||
} | ||
|
||
private func getPastGames(allGames: [Game]) -> [Game] { |
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 idea with this function - use the filter function!
// completion([]) | ||
// } | ||
// } | ||
//} |
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.
For all of the above commented lines, delete if they're unused, or leave a TODO: with some comments about it.
.frame(width: 32, height: 32) | ||
.foregroundStyle(selected ? Constants.Colors.selected : Constants.Colors.iconGray) | ||
// .foregroundStyle(selected ? Constants.Colors.selected : Constants.Colors.iconGray) |
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.
Remove if unused.
.frame(maxWidth: .infinity, alignment: .leading) | ||
} | ||
.padding(.leading, 24) | ||
.padding(.trailing, 24) |
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.
.padding(.trailing, 24) | |
.padding(.trailing, 24) | |
.font(Constants.Fonts.subheader) | ||
Text("Cornell vs. " + game.opponent.rawValue) | ||
.font(Constants.Fonts.header) | ||
.font(Constants.Fonts.medium14) |
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.
.font(Constants.Fonts.medium14) | |
.font(Constants.Fonts.medium14) | |
|
||
HStack() { | ||
Image("Location-g") | ||
.resizable() | ||
.frame(width: 23, height: 26) | ||
.frame(width: 13, height: 19) |
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.
.frame(width: 13, height: 19) | |
.frame(width: 13, height: 19) | |
|
||
HStack() { | ||
Image("Location-g") | ||
.resizable() | ||
.frame(width: 23, height: 26) | ||
.frame(width: 13, height: 19) | ||
Text("Ithaca (Schoellkopf)") |
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.
Text("Ithaca (Schoellkopf)") | |
Text("Ithaca (Schoellkopf)") | |
Text("Ithaca (Schoellkopf)") | ||
Image("Alarm") | ||
.resizable() | ||
.frame(width: 24, height: 24) | ||
.frame(width: 19.78, height: 18.34) |
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.
.frame(width: 19.78, height: 18.34) | |
.frame(width: 19.78, height: 18.34) | |
Text("2") | ||
.font(Constants.Fonts.countdownNum) | ||
Text("days") | ||
.font(Constants.Fonts.gameText) | ||
Text("0") | ||
.font(Constants.Fonts.countdownNum) | ||
Text("hours") | ||
.font(Constants.Fonts.gameText) |
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.
Place empty lines in between views.
Image("Calendar") | ||
.resizable() | ||
.frame(width: 24, height: 24) | ||
Text("Add to Calendar") | ||
.font(Constants.Fonts.buttonLabel) |
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.
Place empty lines in between views.
Button(action: { | ||
// TODO: action | ||
}) { |
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.
Can you use the standard Button format:
Button {
// Action
} label: {
// UI
}
@@ -18,11 +18,19 @@ struct GameTile: View { | |||
// Opponent Logo, Opponent Name | Sport Icon, Sex Icon | |||
HStack(spacing: 8) { | |||
HStack(spacing: 8) { | |||
Image(game.opponent.rawValue) | |||
// Image(game.opponent) |
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.
Remove comment.
@State private var selectedSex : Sex = .Both | ||
@State private var selectedSport : Sport = .All | ||
var paddingMain : CGFloat = 20 | ||
@State private var selectedCardIndex: Int = 0 | ||
@State private var games: [Game] = [] | ||
@State private var allGames: [Game] = [] | ||
@State private var upcomingGames: [Game] = [] | ||
@State private var errorMessage: 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.
Could you make these alphabetical order?
HomeView() | ||
} | ||
|
||
// MARK: Functions |
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.
All of this code would make more sense in a ViewModel, could you move this to the HomeViewModel?
Also, similar idea with the filtering/sorting in my other comment.
Button { | ||
selectedSport = sport | ||
} label: { | ||
FilterTile(sport: sport, selected: sport == selectedSport) |
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.
FilterTile(sport: sport, selected: sport == selectedSport) | |
FilterTile(sport: sport, selected: sport == selectedSport) |
ForEach( | ||
games | ||
) { game in |
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.
Place these on the same line.
.frame(maxWidth: .infinity) | ||
.background(Constants.Colors.white) | ||
.shadow(radius: 6) | ||
|
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.
import SwiftUI | ||
|
||
struct PastGameCard: View { | ||
var game: Game |
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.
var game: Game | |
var game: Game | |
.resizable() | ||
.frame(width: 64, height: 64) |
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.
Add an indent.
AsyncImage(url: URL(string: game.opponent.image)) {image in | ||
image.resizable() | ||
} placeholder: { | ||
Constants.Colors.gray_icons | ||
} | ||
.frame(width: 25, height: 27) | ||
Text(game.opponent.name) | ||
.font(Constants.Fonts.gameTitle) | ||
Spacer() | ||
Image(game.sport.rawValue + "-g") | ||
.resizable() | ||
.frame(width: 30, height: 30) | ||
Image(game.sex.description + "-g") | ||
.resizable() | ||
.frame(width: 25, height: 25) |
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.
Empty lines in between views.
Image("Location-g") | ||
.resizable() | ||
.frame(width: 10, height: 15) | ||
Text("\(game.city), \(game.state)") | ||
.font(Constants.Fonts.gameText) | ||
.foregroundStyle(.gray) | ||
Spacer() | ||
Text(Date.dateToString(date: game.date)) | ||
.font(Constants.Fonts.gameDate) | ||
.foregroundStyle(.gray) |
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.
Empty lines in between views.
@@ -12,20 +12,95 @@ struct PastGameTile: View { | |||
|
|||
|
|||
var body: some View { |
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 is quite a big code section, could you separate these into separate variables?
// arrow | ||
|
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 something that's supposed to be here?
@State private var errorMessage: String? | ||
@State private var pastGames: [Game] = [] | ||
@EnvironmentObject var viewModel: GamesViewModel | ||
|
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.
@State private var selectedSex : Sex = .Both | ||
@State private var selectedSport : Sport = .All | ||
var paddingMain : CGFloat = 20 | ||
@State private var selectedCardIndex: Int = 0 | ||
@State private var games: [Game] = [] | ||
@State private var allGames: [Game] = [] | ||
@State private var errorMessage: String? | ||
@State private var pastGames: [Game] = [] | ||
@EnvironmentObject var viewModel: GamesViewModel |
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.
Alphabetical order please :)
} | ||
} | ||
|
||
#Preview { | ||
PastGameView() | ||
} | ||
|
||
// MARK: Functions |
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.
These functions would go in the view model as well. Then you would call it by calling viewModel.fetchPastGames()
.
Button { | ||
selectedSport = sport | ||
} label: { | ||
FilterTile(sport: sport, selected: sport == selectedSport) |
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.
FilterTile(sport: sport, selected: sport == selectedSport) | |
FilterTile(sport: sport, selected: sport == selectedSport) |
} | ||
}.padding(.top, paddingMain) | ||
} | ||
} |
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.
In general in this view, I see lots of duplicate code that is used elsewhere - could you create files that hold those views so that you can reuse them? e.g. a Carousel file that contains a Carousel view.
} | ||
} | ||
|
||
struct ScoringUpdateCell : View { |
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.
Create another file for this struct.
HStack { | ||
if update.isCornell { | ||
Image("Cornell") | ||
.resizable().frame(width: 32, height: 32) |
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.
.resizable().frame(width: 32, height: 32) | |
.resizable().frame(width: 32, height: 32) |
.padding(.leading, 24) | ||
.padding(.trailing, 24) | ||
.padding(.top, 12) | ||
.padding(.bottom, 12) |
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 use .horizontal and .vertical to make these more concise.
@Binding var selectionIndex: Int | ||
|
||
let itemIndex: Int | ||
private let tabItems = ["schedule", "scoreboard"] |
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.
Indentation is off here.
Image(itemIndex == selectionIndex ? "\(tabItems[itemIndex])-selected" : tabItems[itemIndex]) | ||
.resizable() | ||
.frame(width: 28, height: 28) | ||
.tint(Constants.Colors.gray_icons) | ||
Text(itemIndex == 0 ? "Schedule" : "Scores") | ||
.font(Constants.Fonts.buttonLabel) | ||
.foregroundStyle(itemIndex == selectionIndex ? Constants.Colors.primary_red : Constants.Colors.unselectedText) |
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.
Empty line in between views.
AsyncImage(url: URL(string: game.opponent.image)) { | ||
image in |
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.
Place on the same line.
.resizable() | ||
.frame(width: 24, height: 30) | ||
Text(game.opponent.rawValue) | ||
AsyncImage(url: URL(string: game.opponent.image)) {image in |
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.
AsyncImage(url: URL(string: game.opponent.image)) {image in | |
AsyncImage(url: URL(string: game.opponent.image)) { image in |
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.
Awesome PR Adelynn!! There's a few styling comments I'd like you to address, as well as making some functions that filter/sort games, etc more concise. Please let me know if you have any specific questions :)
Overview
Integrated backend into the app using Apollo-client and fixed filter and icon-rendering issues.
Changes
Change 1
Set up GraphQL schema and generated queries.
Change 2
Implemented functions in NetworkManager
Change 3
Fixed filtering functions in HomeView and PastGameView
Change 4
Updated icon image names
Change 5
Modified Game struct, added new Team struct, and implemented decoding initializers for all structs in model Game