Skip to content
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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Implement giveaway popup #55

wants to merge 6 commits into from

Conversation

belle-hu
Copy link
Contributor

@belle-hu belle-hu commented Apr 15, 2024

Overview

Implement giveaway popup. Shoutout to Caitlyn for helping me with networking a lot.

Changes Made

Models

  • Implemented User Model
  • Implemented Giveaway Model

GraphQL files

  • Created GiveawayQueries.graphql
  • Created GiveawayMutations.graphql

ViewModels

  • Implemented GiveawayViewModel to include networking functions

Views

  • Implemented GiveawayPopup
  • Implemented GiveawayResponse
  • Revised HomeView to include these popups

Other Changes

  • Revised Array+Extension, but not sure if that was necessary tbh.

Screenshots (optional)

TODO
Before After
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-04-14.at.22.40.40.mp4

Copy link
Collaborator

@vinnie4k vinnie4k left a 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 {
Copy link
Collaborator

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 {
Copy link
Collaborator

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.

Comment on lines +1 to +23
{
"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
}
}
Copy link
Collaborator

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.

Copy link
Collaborator

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.

Comment on lines +1 to +23
{
"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
}
}
Copy link
Collaborator

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.

Comment on lines +145 to +152

extension Array where Element == User {

init(_ users: [UserFields]) {
self.init(users.map(User.init))
}

}
Copy link
Collaborator

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.

Copy link
Collaborator

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.

Comment on lines +16 to +19
@State var popupIsPresented: Bool = false
@EnvironmentObject var locationManager: LocationManager
@StateObject private var viewModel = ViewModel()
@State var popupSubmitted: Bool = false
Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants