-
Notifications
You must be signed in to change notification settings - Fork 69
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
Get exercises from backend on DOJO exercises page #2335
Get exercises from backend on DOJO exercises page #2335
Conversation
@bryanjenningz is attempting to deploy a commit to the c0d3-prod Team on Vercel. A member of the Team first needs to authorize it. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
challengeName: exercise?.module.name!, | ||
problem: exercise?.description!, | ||
answer: exercise?.answer!, | ||
explanation: exercise?.explanation! |
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.
Using the ?.
and !
operators here isn't ideal.
The reason why we need to do this is because the type definition for the exercises is exercises: [Exercise]!
instead of exercises: [Exercise!]!
: https://github.com/garageScript/c0d3-app/blob/master/graphql/typeDefs.ts#L9
Would it be better to change the schema to exercises: [Exercise!]!
so that we don't have to worry about null values?
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 thought we should do that once there are some exercises. Wouldn't it throw an error when getting the exercises if there are none?
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.
If we changed the type definition to exercises: [Exercise!]!
and if there are no exercises (i.e. it returns an empty array []
), then it shouldn't throw an error because []
is the type [Exercise!]!
.
It would only throw an error if there were null values in the array (e.g. [null, null]
).
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 added a pull request for if we want to make this change: #2336
Codecov Report
@@ Coverage Diff @@
## master #2335 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 180 181 +1
Lines 3177 3179 +2
Branches 850 850
=========================================
+ Hits 3177 3179 +2
|
The graphql/index.tsx file has the same diff in this PR: https://github.com/garageScript/c0d3-app/pull/2330/files |
Do you want me to close that pull request since it's overlapping this one or do you want me to merge that pull request so that this pull request has a smaller diff? |
@@ -0,0 +1,31 @@ | |||
import { gql } from '@apollo/client' | |||
|
|||
const GET_EXERCISES = gql` |
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 wasn't sure what to call this query. It does more than just get the exercises - it gets the lessons and alerts too which are used on the DOJO exercises page.
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.
GET_DOJO_EXERCISES
maybe, but it's longer.
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.
Yeah, or maybe even something like GET_DOJO_EXERCISES_PAGE_DATA
since we're getting all the data we need for the DOJO exercises page. Maybe GET_DOJO_EXERCISES
is fine. Maybe we should also change GET_FLAGGED_EXERCISES
to be GET_FLAGGED_DOJO_EXERCISES
so that the naming convention is consistent.
I think it's better to keep this file out of this PR. |
I also added a new query in this pull request which changed this file as well. Should I move the new query I added to a separate pull request so that this file diff is not in this pull request? |
It's better if we merge the other PR, then have a new diff with that change only. |
Sounds good. |
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'll merge this pull request since it's mostly good and everything works. We can fix the exercises type definition to be |
This pull request doesn't affect any user-facing pages since there are no direct links to the DOJO exercises page.
This pull request makes it so the DOJO exercises page gets the exercises from the backend instead of relying on mock data on the frontend.
How to test:
The exercise page currently doesn't send any mutations to the backend. We will add mutations to update the user's answers to the exercises on the backend in a subsequent pull request.
This pull request is a part of #2253