-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat(surveys): Single Choice question results #17923
Conversation
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.
will test it out shortly, but this is looking great nice job! :D
@@ -314,7 +358,15 @@ export const surveyLogic = kea<surveyLogicType>([ | |||
{}, | |||
{ | |||
loadSurveyRatingResultsSuccess: (state, { payload }) => { |
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.
Where is this being used btw? I can't find any references to it
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.
This sets surveySingleChoiceResultsReady
- an object of booleans where the key represents the index of the question for which we are fetching results.
Each chart does its own fetching, so we need to store the loading state separately for each. Same goes for the results, which we store in surveySingleChoiceResults
- also an object of question indices.
Looks good! Let's maybe double the height of the pie? And cut the padding between legend items by 50%. Then we can stack more items in a single column before having to split to 2 columns. |
@corywatilo sizes adjusted: |
{}, | ||
{ | ||
loadSurveySingleChoiceResultsSuccess: (state, { payload }) => { | ||
return { ...state, [`${payload?.questionIndex}`]: true } |
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 think we need some error handling here since that should be the only case when questionIndex doesn't exist in the payload and this incorrectly adds 'true' to undefined.
(and a more general error state for when the hogql query fails, vanishing / infinite loading the graph is suboptimal) - but ok to do in a follow up
@corywatilo