Draft: Add auto-sum-to-100 button to multi choice questions #87
Conversation
|
@slowSort is attempting to deploy a commit to the Sage Team on Vercel. A member of the Team first needs to authorize it. |
| import MultiChoiceQuestion from "./MultiChoiceQuestion" | ||
| import { PredictProvider } from "../PredictProvider" | ||
|
|
||
| describe("MultiChoiceQuestion", () => { |
There was a problem hiding this comment.
Will add more tests when test setup is fixed
| @@ -60,24 +77,34 @@ export default function MultiChoiceQuestion({ | |||
| /> | |||
| ))} | |||
|
|
|||
| {fields.length < MAX_OPTIONS && ( | |||
There was a problem hiding this comment.
Might be wrong, but I think this is now handled by the addOption callback, decided to remove but let me know if I've missed something
| Add option | ||
| </button> | ||
|
|
||
| { | ||
| <button |
There was a problem hiding this comment.
Github has diffed this very strangely but if you take a look at the code I'm just adding a new button here along with some styling
| @@ -0,0 +1,41 @@ | |||
| import { OptionType } from "../components/predict-form/PredictProvider" | |||
|
|
|||
| export function normalizeOptionsToHundred(options: OptionType[]) { | |||
There was a problem hiding this comment.
considered naming getNormalizeOptionsToHundred to make it clearer we're not mutating – let me know what you think. I'm usually a big "get prefixer" but I can see the good in both approaches
| const optionsCopy = [...options] | ||
| const optionsWithForecasts = optionsCopy.filter( | ||
| ({ forecast }) => forecast !== undefined && !Number.isNaN(forecast), | ||
| ) as Array<OptionType & { forecast: number }> |
There was a problem hiding this comment.
Typing forecast here means that I don't have to assert it to be a value in each of my filters
There was a problem hiding this comment.
Alternatively, I see in PredictProvider.tsx that optionSchema can be NaN. Any reason why? If we could get rid of that then we could at least get rid of the isNaN check – not urgent though
Pull Request: Add auto-sum-to-100 button to multi choice questions
Related Issue
Closes [#82] – please check for continued discussion in the issue while this is in draft
Changes Made
Testing
TODO: Once test suite no longer failing