Zain/implement highlight UI v2 #42
Conversation
MrPeterss
left a comment
MrPeterss
left a comment
There was a problem hiding this comment.
Nice work zain, just a couple of comments!
| } | ||
| } | ||
|
|
||
| var sport: Sport{ |
There was a problem hiding this comment.
can we add a space between the "t" and "{"
| } | ||
|
|
||
| var sport: Sport{ | ||
| switch self{ |
There was a problem hiding this comment.
same here space before the "{"
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
i like both of the approaches with the complations and the async/await
| .resizable() | ||
| .frame(width: 9.87, height: 18.57) | ||
| } | ||
| Spacer() |
|
|
||
| struct NoHighlightView: View { | ||
| var body: some View { | ||
| VStack { |
There was a problem hiding this comment.
keep the spaces between the vstack children
| ) | ||
| } | ||
|
|
||
|
|
| .onChange(of: viewModel.selectedSport) { _, _ in | ||
| viewModel.filter() | ||
| } | ||
|
|
| NavigationLink(destination: | ||
| DetailedHighlightsView(title: title, highlightScope: scope) | ||
| .environmentObject(viewModel)) | ||
| { |
There was a problem hiding this comment.
either put the "{" above on line 92 or move one of the closing ")" on line 93
angelinaa-chen
left a comment
angelinaa-chen
left a comment
There was a problem hiding this comment.
Nice PR Zain! A couple comments, and maybe let's wait until back-end pushes their changes to test the sportsType filtering before merging. Otherwise, looks amazing :) great work!!
There was a problem hiding this comment.
Small nit: HighlightStar is misspelled (missing the t)
| "scale" : "1x" | ||
| }, | ||
| { | ||
| "filename" : "kid_star (1).png", |
There was a problem hiding this comment.
The filename and the name of the actual image file differ -- could we have them uniform just in case?
| @@ -0,0 +1,187 @@ | |||
| // | |||
There was a problem hiding this comment.
This is a general comment, but just leaving this here. The reason the tab bar wasn't showing when clicking the highlights tab was because the current image is called "Highlight-selected" instead of "highlight-selected". Changing that for me allowed it to work again. :)
| .fontWeight(.bold) | ||
| .foregroundColor(Constants.Colors.white) | ||
| .underline() | ||
| // Text(article.source) |
There was a problem hiding this comment.
If this comment isn't needed anymore, could we go ahead and delete it? If not I think adding a comment as to what it might be used for in the future might be nice!
| init(from gqlArticle: ArticlesQuery.Data.Article) { | ||
| self.id = gqlArticle.id ?? UUID().uuidString | ||
| self.image = gqlArticle.image ?? "" | ||
| self.title = gqlArticle.title |
There was a problem hiding this comment.
Not sure if this will ever happen, but could we also put a fallback value for the title and url since the Article properties aren't optionals? An empty string would work as a default
Added
HighlightsViewModelwith loading, filtering, and clean separation of highlights into structured lists for the views.Added
NoHighlightViewImplemented networking for fetching articles and YouTube videos.
(This looks like a big PR, but the lines of code mostly come from
HighlightsViewModel, which copies a lot of its structure from already existingGamesViewModelandPastGameViewModelNetworking:
I originally implemented both fetches using separate completion handlers, but since both are always needed, I switched to an async/await setup and grouped the calls.
This is my first time using async/await in Swift, so I’d appreciate extra attention on that part to make sure the approach is correct and safe.
Next Steps:
Logic:
Style
Other