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.

Sign up for GitHub

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

Make DOJO exercises not nullable and fix React key bug #2336

Merged
merged 2 commits into from
Sep 21, 2022
Merged

Make DOJO exercises not nullable and fix React key bug #2336

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8fac01b
Make DOJO exercises not nullable, fix React key bug
bryanjenningz Sep 21, 2022
3a6f6a6
Make DOJO exercises tests pass after fixing React key bug
bryanjenningz Sep 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion __dummy__/getExercisesData.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ const getExercisesData: GetExercisesQuery = {
},
description: '```\nlet a = 1\na -= 2\n// what is a?\n```',
answer: '-1',
explanation: '`a -= 2` is a shorter way to write `a = a - 2`'
explanation: null
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adding a null exercise explanation to the mock exercise data so we can get full test coverage.

}
]
}
Expand Down
3 changes: 2 additions & 1 deletion __tests__/pages/exercises/[lessonSlug].test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,11 @@ describe('Exercises page', () => {
// Previous button is not in the document on the first exercise.
expect(queryByRole('button', { name: 'PREVIOUS' })).not.toBeInTheDocument()

const skipButton = getByRole('button', { name: 'SKIP' })
let skipButton = getByRole('button', { name: 'SKIP' })
fireEvent.click(skipButton)
expect(queryByRole('button', { name: 'PREVIOUS' })).toBeInTheDocument()

skipButton = getByRole('button', { name: 'SKIP' })
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason why we need to reassign skipButton to the current skip button in the DOM is because we are using a React key prop which makes it so the Exercise component rerenders.

fireEvent.click(skipButton)
// Skip button should not be in the document because we're on the last exercise now.
expect(queryByRole('button', { name: 'SKIP' })).not.toBeInTheDocument()
Expand Down
8 changes: 4 additions & 4 deletions graphql/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ export type Query = {
__typename?: 'Query'
alerts: Array<Alert>
allUsers?: Maybe<Array<Maybe<User>>>
exercises: Array<Maybe<Exercise>>
exercises: Array<Exercise>
getLessonMentors?: Maybe<Array<Maybe<User>>>
getPreviousSubmissions?: Maybe<Array<Submission>>
isTokenValid: Scalars['Boolean']
Expand Down Expand Up @@ -838,7 +838,7 @@ export type GetExercisesQuery = {
name: string
lesson: { __typename?: 'Lesson'; slug: string }
}
} | null>
}>
}

export type GetFlaggedExercisesQueryVariables = Exact<{ [key: string]: never }>
Expand All @@ -852,7 +852,7 @@ export type GetFlaggedExercisesQuery = {
__typename?: 'Module'
lesson: { __typename?: 'Lesson'; title: string }
}
} | null>
}>
}

export type LessonMentorsQueryVariables = Exact<{
Expand Down Expand Up @@ -1780,7 +1780,7 @@ export type QueryResolvers<
ContextType
>
exercises?: Resolver<
Array<Maybe<ResolversTypes['Exercise']>>,
Array<ResolversTypes['Exercise']>,
ParentType,
ContextType
>
Expand Down
2 changes: 1 addition & 1 deletion graphql/typeDefs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ export default gql`
session: Session!
allUsers: [User]
modules: [Module]!
exercises: [Exercise]!
exercises: [Exercise!]!
getLessonMentors(lessonId: Int!): [User]
userInfo(username: String!): Session
isTokenValid(cliToken: String!): Boolean!
Expand Down
9 changes: 5 additions & 4 deletions pages/exercises/[lessonSlug].tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,10 @@ const Exercises: React.FC<QueryDataProps<GetExercisesQuery>> = ({
const currentExercises = exercises
.filter(exercise => exercise?.module.lesson.slug === slug)
.map(exercise => ({
challengeName: exercise?.module.name!,
problem: exercise?.description!,
answer: exercise?.answer!,
explanation: exercise?.explanation!
challengeName: exercise.module.name,
problem: exercise.description,
answer: exercise.answer,
explanation: exercise.explanation || ''
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exercise explanations are nullable so we have to handle it here by defauting to an empty string, which works fine and looks good (see pull request description for a picture of what it looks like).

}))

const exercise = currentExercises[exerciseIndex]
Expand All @@ -66,6 +66,7 @@ const Exercises: React.FC<QueryDataProps<GetExercisesQuery>> = ({
<Layout title={currentLesson.title}>
{exercise ? (
<Exercise
key={exerciseIndex}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This line fixes a small React key bug which previously would make it possible to press "Show Answer" (to show the current exercise answer) then "Skip" (to skip to the next exercise) which caused the next exercise's answer to be shown.

exercise={exercise}
setExerciseIndex={setExerciseIndex}
lessonTitle={currentLesson.title}
Expand Down