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

Feature/harsh jenny/pre session form #70

Merged
merged 30 commits into from
Apr 27, 2023

Conversation

HarshGurnani
Copy link
Contributor

@HarshGurnani HarshGurnani commented Mar 12, 2023

Tracking Info

Resolves #66

Make sure your branch name conforms to: <feature|staging|bugfix|...>/<username>/<3-4 word description separated by dashes>. Otherwise, please rename your branch and create a new PR.

Changes

Added the model and view model for Question, which will be used for the Pre and Post Session Forms

Testing

How did you confirm your changes worked?

NA

Confirmation of Change

QuestionModel

QuestionViewModel

Missed.Session.Form.mov
Post.Session.Form.mov

Copy link
Contributor

@christen03 christen03 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i may be mistaken, but i don't think we're planning to use a route for the questions themselves as they're going to be simply written in? but i'm not 100% sure i'll let aman confirm this

}

func fetchQuestions() async throws -> [Question] {
let url = URL(string: "http://localhost:3000/notes/")!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this to get the actual questions themselves? i don't think we're planning to host the questions in the backend, i think they're just gonna be hard coded in.

unless this is to get the answers? if so we need an id after the notes as well.

Copy link
Collaborator

@AmanKAggarwal AmanKAggarwal Mar 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are planning to host questions on the backend haha. We can discuss this in our meeting but this is the response the GET notes route should return

GET Notes
URL - /notes/<id>
Response Format
[
    {
        question: "Question?",
        type: "text",
        answer: "",
        id: "the hashed ID 1"
    }, 
    {
        question: "Question?",
        type: "bullet",
        answer: [],
        id: "the hashed ID 2"
    }
]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can all backend interactions be consolidated into one API service class? #69 adds an APIServices file, perhaps use something like that

viewModel.questionList = viewModel.loadTestData()
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dang yall popped off fr

}
.buttonStyle(FilledInButtonStyle(disabled: false))
.padding(.bottom, 32)
.frame(width: UIScreen.main.bounds.width * 0.575)
Copy link
Collaborator

@AmanKAggarwal AmanKAggarwal Mar 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use GeometryReader instead of UIScreen.

  1. GeometryReader is SwiftUI's native way of getting size of view (checkout ProgresBarComponent and https://developer.apple.com/documentation/swiftui/geometryreader)
  2. UIScreen.main is deprecated - https://developer.apple.com/documentation/uikit/uiscreen/1617815-main
  3. UIScreen is UIKit's way of getting screen size so less preferred for SwiftUI

.padding(.trailing, 16)
.padding(.bottom, 32)
} else if currQuestion.type == "bullet" {

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not render currQuestion.answerBullet in this case?

@AmanKAggarwal
Copy link
Collaborator

Model + ViewModel for Questions look good. Left a few comments for the Summary Page

@christen03 we do want to get our questions from the backend. That way if ALUM wants to change the question in future, they can just change it in the backend instead of Pushing an Update for all users on the frontend

Copy link
Collaborator

@AmanKAggarwal AmanKAggarwal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff! Looking forward to the use of this model and viewmodel

Copy link
Contributor

@christen03 christen03 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

really great job guys :)

}

func fetchQuestions() async throws -> [Question] {
let url = URL(string: "http://localhost:3000/notes/")!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can all backend interactions be consolidated into one API service class? #69 adds an APIServices file, perhaps use something like that

Comment on lines 12 to 14
struct PreSessionConfirmationScreen: View {
@ObservedObject var viewModel: QuestionViewModel
@Environment(\.dismiss) var dismiss
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future, try to include a screenshot of any frontend pages/components that you created or updated in the PR. That's what the "Confirmation of Change" section in the template is for

@AmanKAggarwal
Copy link
Collaborator

Agreed with @wllmwu's comments

  1. This PR is more for getting the ViewModel and Model reviewed so no screenshots for screen was added but can add ss for the summary page (not necessary rn)
  2. Yup, we will keep all backend interactions in the service class. Rn the interaction is not working and its just there as a placeholder

@AmanKAggarwal
Copy link
Collaborator

Fixed the issue regarding the app crash when done was hit. It was because we had made the function add a state.

There are still a couple bugs -

  1. When you press the back button, questions are reloaded so duplicated
  2. There is a title which is not on figma

image

@jennymar jennymar force-pushed the feature/HarshJenny/PreSessionForm branch from 7c6522d to 81ee69d Compare April 27, 2023 03:06
@AmanKAggarwal AmanKAggarwal force-pushed the feature/HarshJenny/PreSessionForm branch from 171cbdb to 1e35713 Compare April 27, 2023 12:47
@HarshGurnani
Copy link
Contributor Author

Merging pre/post session flow

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.

Pre/Post Form - Create QuestionModel + QuestionsViewModel
5 participants