-
Notifications
You must be signed in to change notification settings - Fork 10
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
Draft: Add auto-sum-to-100 button to multi choice questions #87
base: main
Are you sure you want to change the base?
Changes from all commits
dd02a1a
45cf930
def31eb
acd4215
c9ef26c
33f650d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
import { render, screen, fireEvent } from "@testing-library/react" | ||
import MultiChoiceQuestion from "./MultiChoiceQuestion" | ||
import { PredictProvider } from "../PredictProvider" | ||
|
||
describe("MultiChoiceQuestion", () => { | ||
const mockQuestion = { | ||
id: "test-question", | ||
type: "multiple-choice", | ||
title: "Test Question", | ||
options: [ | ||
{ id: "option-1", text: "Option 1" }, | ||
{ id: "option-2", text: "Option 2" }, | ||
{ id: "option-3", text: "Option 3" }, | ||
], | ||
} | ||
|
||
const defaultProps = { | ||
question: mockQuestion, | ||
predictions: {}, | ||
onChange: jest.fn(), | ||
onSubmit: jest.fn(), | ||
highlightResolveBy: false, | ||
} | ||
|
||
const renderWithProvider = (props = {}) => { | ||
return render( | ||
<PredictProvider> | ||
<MultiChoiceQuestion {...defaultProps} {...props} /> | ||
</PredictProvider>, | ||
) | ||
} | ||
|
||
it("automatically adjusts values to maintain 100 total", () => { | ||
const onChange = jest.fn() | ||
renderWithProvider({ onChange }) | ||
|
||
const inputs = screen.getAllByRole("spinbutton") | ||
fireEvent.change(inputs[0], { target: { value: "60" } }) | ||
fireEvent.change(inputs[1], { target: { value: "30" } }) | ||
|
||
expect(onChange).toHaveBeenLastCalledWith({ | ||
"option-1": 60, | ||
"option-2": 30, | ||
"option-3": 10, | ||
}) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,9 @@ import { PredictButton } from "../PredictButton" | |
import { QuestionOption } from "../QuestionOption" | ||
import { ResolveBy } from "../ResolveBy" | ||
import { QuestionTypeProps } from "./question-types" | ||
import { OptionType } from "../PredictProvider" | ||
import { normalizeOptionsToHundred } from "../../../lib/_utils_multiple-choice" | ||
import { formatDecimalNicely } from "../../../lib/_utils_common" | ||
|
||
export default function MultiChoiceQuestion({ | ||
small, | ||
|
@@ -17,7 +20,7 @@ export default function MultiChoiceQuestion({ | |
onSubmit, | ||
highlightResolveBy, | ||
}: QuestionTypeProps) { | ||
const { trigger, control } = useFormContext() | ||
const { trigger, control, watch, setValue } = useFormContext() | ||
|
||
const MIN_OPTIONS = 2 | ||
const MAX_OPTIONS = 100 | ||
|
@@ -40,6 +43,20 @@ export default function MultiChoiceQuestion({ | |
[remove], | ||
) | ||
|
||
const options = watch("options") as OptionType[] | ||
const normalizeToHundred = useCallback(() => { | ||
const normalizedOptions = normalizeOptionsToHundred(options) | ||
|
||
normalizedOptions.forEach((option, index) => { | ||
setValue( | ||
`options.${index}.forecast`, | ||
formatDecimalNicely(option.forecast || 0, 0), | ||
) | ||
}) | ||
|
||
void trigger("options") | ||
}, [options, setValue, trigger]) | ||
|
||
return ( | ||
<div | ||
className={`flex flex-col gap-4 flex-wrap justify-between ${embedded ? "flex-col" : "flex-row"}`} | ||
|
@@ -60,24 +77,34 @@ export default function MultiChoiceQuestion({ | |
/> | ||
))} | ||
|
||
{fields.length < MAX_OPTIONS && ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
<div className="flex flex-row justify-between items-center gap-2"> | ||
<div className="flex-grow"> | ||
<div className="flex flex-row justify-between items-center gap-2"> | ||
<div className="flex-grow flex gap-2"> | ||
<button | ||
onClick={(e) => { | ||
e.preventDefault() | ||
addOption() | ||
}} | ||
className="btn btn-sm text-neutral-500" | ||
> | ||
<PlusIcon className="w-5 h-5" /> | ||
Add option | ||
</button> | ||
|
||
{ | ||
<button | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
onClick={(e) => { | ||
e.preventDefault() | ||
addOption() | ||
normalizeToHundred() | ||
}} | ||
className="btn btn-sm text-neutral-500" | ||
> | ||
<PlusIcon className="w-5 h-5" /> | ||
Add option | ||
Sum to 100% | ||
</button> | ||
</div> | ||
<div className=""></div> | ||
<div className=""></div> | ||
} | ||
</div> | ||
)} | ||
<div className=""></div> | ||
<div className=""></div> | ||
</div> | ||
|
||
<FormCheckbox | ||
onSubmit={onSubmit} | ||
|
@@ -86,7 +113,7 @@ export default function MultiChoiceQuestion({ | |
helpText="If selected, you can resolve multiple options to YES. Otherwise, you can only resolve a single option to YES (and an OTHER option is added by default)." | ||
labelClassName="text-sm" | ||
onChange={() => { | ||
void trigger("options") // needed because our superRefine rule for summing to 100% isn't automatically revalidated otherwise | ||
void trigger("options") | ||
}} | ||
/> | ||
</div> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
import { OptionType } from "../components/predict-form/PredictProvider" | ||
|
||
export function normalizeOptionsToHundred(options: OptionType[]) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
const optionsWithoutForecasts = optionsCopy.filter( | ||
({ forecast }) => forecast === undefined || Number.isNaN(forecast), | ||
) | ||
|
||
const allOptionsHaveForecasts = | ||
optionsWithForecasts.length === optionsCopy.length | ||
const totalPercentage = optionsWithForecasts.reduce( | ||
(acc, option) => acc + option.forecast, | ||
0, | ||
) | ||
|
||
if (allOptionsHaveForecasts) { | ||
optionsWithForecasts.forEach((option, index) => { | ||
const scaledValue = (option.forecast * 100) / totalPercentage | ||
optionsCopy[index] = { | ||
...option, | ||
forecast: scaledValue, | ||
} | ||
}) | ||
} else { | ||
const remainingMass = Math.max(0, 100 - totalPercentage) | ||
const massPerOption = remainingMass / optionsWithoutForecasts.length | ||
|
||
optionsWithoutForecasts.forEach((option) => { | ||
const originalIndex = options.findIndex((o) => o === option) | ||
optionsCopy[originalIndex] = { | ||
...option, | ||
forecast: massPerOption, | ||
} | ||
}) | ||
} | ||
|
||
return optionsCopy | ||
} |
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 add more tests when test setup is fixed