-
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
Implement giveaway popup #55
base: main
Are you sure you want to change the base?
Conversation
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.
Great job on this Belle! I highly recommend that you and @caitlynjin take a look at this PR and make changes following it. I left a few comments pointing out some issues that I saw, but ultimately, the PR should be able to explain things if my comments do not make sense.
Request me again once you've made the appropriate changes. You can also copy the code that I wrote for the sake of time as long as you understand why I do the things that I'm doing.
import UpliftAPI | ||
|
||
/// Model representing a giveaway. | ||
struct Giveaway: Hashable { |
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 entire model is not needed because frontend does not do anything with it.
import UpliftAPI | ||
|
||
/// Model representing a user. | ||
struct User: Hashable { |
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 model is also not needed (at this moment) because frontend does not do anything with it yet.
{ | ||
"images" : [ | ||
{ | ||
"filename" : "Giveaway modal.png", | ||
"idiom" : "universal", | ||
"scale" : "1x" | ||
}, | ||
{ | ||
"filename" : "Giveaway modal 1.png", | ||
"idiom" : "universal", | ||
"scale" : "2x" | ||
}, | ||
{ | ||
"filename" : "Giveaway modal 2.png", | ||
"idiom" : "universal", | ||
"scale" : "3x" | ||
} | ||
], | ||
"info" : { | ||
"author" : "xcode", | ||
"version" : 1 | ||
} | ||
} |
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 images should follow a proper naming convention. On Figma, you can export an image with 2x and 3x scaling. You would then append @2x
and @3x
to the end of the file name. Also, the file name should contain underscores instead of spaces. There should also be no capital letters.
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 not a good idea to use images that contain text. This does not scale properly with different phone sizes. The only image that you need is the background image as well as the app logo. These images should also NOT have a corner radius so that the frontend code can customize it.
{ | ||
"images" : [ | ||
{ | ||
"filename" : "Frame (3).png", | ||
"idiom" : "universal", | ||
"scale" : "1x" | ||
}, | ||
{ | ||
"filename" : "Frame (4).png", | ||
"idiom" : "universal", | ||
"scale" : "2x" | ||
}, | ||
{ | ||
"filename" : "Frame (5).png", | ||
"idiom" : "universal", | ||
"scale" : "3x" | ||
} | ||
], | ||
"info" : { | ||
"author" : "xcode", | ||
"version" : 1 | ||
} | ||
} |
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 regarding the name of the files. Also, it's a good idea to make the names more descriptive.
|
||
extension Array where Element == User { | ||
|
||
init(_ users: [UserFields]) { | ||
self.init(users.map(User.init)) | ||
} | ||
|
||
} |
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.
We won't be needing this.
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.
We should have this ViewModel be a ViewModel for the MainView
. Take a look at how I wrote MainViewModel
in this PR.
@State var popupIsPresented: Bool = false | ||
@EnvironmentObject var locationManager: LocationManager | ||
@StateObject private var viewModel = ViewModel() | ||
@State var popupSubmitted: Bool = false |
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 @State properties should be marked as private
.
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.
Take a look at how I wrote GiveawayPopup
in this PR. This view shouldn't be owning any properties, as the parent view should be the one creating the property and passing it to this view with @binding. You will also need to rewrite the code since we are no longer using the image itself as the popup view. Feel free to copy what I wrote, but it's important that you understand the code. Please ask questions if you are confused about my code.
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 this to one single GiveawayPopup
view since only the state of the the view changes. You don't want to replace it with a completely new view because there are things about the old view that should be kept consistent.
Overview
Implement giveaway popup. Shoutout to Caitlyn for helping me with networking a lot.
Changes Made
Models
User
ModelGiveaway
ModelGraphQL files
GiveawayQueries.graphql
GiveawayMutations.graphql
ViewModels
GiveawayViewModel
to include networking functionsViews
GiveawayPopup
GiveawayResponse
HomeView
to include these popupsOther Changes
Array+Extension
, but not sure if that was necessary tbh.Screenshots (optional)
TODO
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-04-14.at.22.40.40.mp4