Td 2500 render results #361
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request implements support for generic surveys/assessments in the Traitify widget system. The implementation adds a complete generic survey flow including taking the survey, submitting responses, and viewing results.
- Adds generic assessment query handling and GraphQL integration
- Implements complete generic survey UI components with progress tracking, question sets, and modal instructions
- Creates comprehensive results display with scoring, breakdown, and individual question analysis
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/recoil/assessment.js | Adds generic assessment query and integrates it into the assessment selector |
| src/lib/graphql/index.js | Exports new generic GraphQL module |
| src/lib/graphql/generic.js | Defines GraphQL queries and mutations for generic surveys |
| src/components/survey/index.js | Adds generic survey routing |
| src/components/survey/generic/* | Complete generic survey implementation with styling, responses, question sets, and container |
| src/components/results/index.js | Adds generic results routing |
| src/components/results/generic/* | Complete results display with header, scoring, breakdown, and question analysis |
| src/components/common/modal/index.js | Adds containerClass prop for custom modal styling |
| public/index.js | Adds generic survey setup and creation functionality |
| package.json | Version bump to 3.8.0 |
Comments suppressed due to low confidence (4)
src/components/results/generic/header.js:1
- The import should use 'PropTypes' with a capital P to match the standard naming convention used throughout the codebase.
import propTypes from "prop-types";
src/components/results/generic/header.js:22
- Should use 'PropTypes' instead of 'propTypes' to match the import and standard convention.
profile: propTypes.shape({
src/components/results/generic/header.js:26
- Should use 'PropTypes' instead of 'propTypes' to match the import and standard convention.
assessment: propTypes.shape({
src/lib/recoil/assessment.js
Outdated
| } | ||
|
|
||
| const assessment = response.data.genericSurveyQuestions; | ||
| if(!assessment?.length || !assessment?.completedAt) { return assessment; } |
There was a problem hiding this comment.
The condition !assessment?.length is checking for an array length property on what should be an assessment object. Based on the API response structure, this should likely check !assessment?.survey?.questionSets?.length or similar property instead.
| if(!assessment?.length || !assessment?.completedAt) { return assessment; } | |
| if(!assessment?.survey?.questionSets?.length || !assessment?.completedAt) { return assessment; } |
|
|
||
| http.post(graphQL.generic.path, {query, variables}).then(({data, errors}) => { | ||
| if(!errors && data.genericAssessmentResult) { | ||
| console.log("Generic assessment result data:", data.genericAssessmentResult); |
There was a problem hiding this comment.
Debug console.log statements should not be left in production code. This should be removed or replaced with proper logging.
tomprats
left a comment
tomprats
left a comment
There was a problem hiding this comment.
Overall, the survey looks really nice! Couple of things required for that:
- translations
- css - keeping it shallow, using theme helper for colors, alphabetize properties!
I haven't been able to render the results, I get the following error in the generic survey component (feel free to follow up in slack about this):
TypeError: can't access property "survey", assessment is null
tomprats
left a comment
tomprats
left a comment
There was a problem hiding this comment.
Doing a quick grep to see where cognitive is that's missing generic, I found these:
- ./src/lib/hooks/use-skip-assessment.js - Are there endpoints for skipping generic?
- ./src/lib/common/order-from-query.js - Recommendation, also using it in the recoil query
Let me discuss with Ryan |
|
@tomprats let's wrap this up. Skipping and recommendation logic will be handled in another PR later |
src/components/index.js
Outdated
| Survey: { | ||
| Cognitive: CognitiveSurvey, | ||
| Container: Survey, | ||
| GenericSurvey, |
There was a problem hiding this comment.
To be consistent let's make this Generic: GenericSurvey,
There was a problem hiding this comment.
And to add the results as well at Results.Generic
There was a problem hiding this comment.
Still need using Generic: GenericSurvey and also adding the results component
tomprats
left a comment
tomprats
left a comment
There was a problem hiding this comment.
These comments are all relatively minor, but the main things left are:
- mobile - several things break out of their containers on smaller screen sizes
- tests
src/components/index.js
Outdated
| Survey: { | ||
| Cognitive: CognitiveSurvey, | ||
| Container: Survey, | ||
| GenericSurvey, |
There was a problem hiding this comment.
Still need using Generic: GenericSurvey and also adding the results component
tomprats
left a comment
tomprats
left a comment
There was a problem hiding this comment.
Awesome work! Looks great!
Just want to make sure the generic service is out in prod before merging this. We also want to make sure the translations are in Contentful so next time we sync they'll still come over (and hopefully will bring more translations)
Code change