-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feature/select single questions #24
Conversation
barshathakuri
commented
Aug 16, 2023
•
edited
Loading
edited
- Detail Change
- Add Single and Multiple question type
- Add Pillars options in both questions type
3d7623c
to
20134c1
Compare
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.
Also, let's fix the typecheck and lint issues.
<MultiSelectInput | ||
keySelector={keySelector} | ||
label="Country" | ||
labelSelector={labelSelector} | ||
name="test" | ||
onChange={function noRefCheck() { }} | ||
options={[]} | ||
value={[]} | ||
/> |
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.
The preview for this question type should be a list of options with a checkbox instead of MultiSelectInput.
limit: PAGE_SIZE, | ||
offset: 0, |
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.
These variables are not called in query and hence is not required here.
}, | ||
); | ||
const initialFormValue: FormType = { | ||
type: 'SELECT_ONE' as QuestionTypeEnum, |
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.
type: 'SELECT_ONE' as QuestionTypeEnum, | |
type: 'SELECT_MULTIPLE' as QuestionTypeEnum, |
} = useQuery<PillarsQuery, PillarsQueryVariables>( | ||
PILLARS, | ||
{ | ||
variables: pillarsVariables, |
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.
Let's add this:
variables: pillarsVariables, | |
skip: isNotDefined(pillarsVariables), | |
variables: pillarsVariables, |
} | ||
); | ||
|
||
const pillarsOptions = pillarsResponse?.private?.projectScope?.groups.items || []; |
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.
We should also include the hierarchy of the options. For eg: For a nested group, let's say, 'Mammals' inside a group 'Animals', the option should say: 'Animals > Mammals'.
instead of currently shown options: 'Animals' and 'Mammals'.
error={fieldError?.hint} | ||
onChange={setFieldValue} | ||
/> | ||
<>Options</> |
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.
We can remove this for now if it's not ready.
import styles from './index.module.css'; | ||
import SelectMultipleQuestionPreview from '#components/SelectMultipleQuestionsPreview'; | ||
|
||
const CREATE_TEXT_QUESTION = gql` |
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.
The query should be named something else like CREATE_MULTIPLE_SELECTION_QUESTION
. Let's not use misleading names.
triggerQuestionCreate, | ||
{ loading: createQuestionPending }, | ||
] = useMutation<CreateTextQuestionMutation, CreateTextQuestionMutationVariables>( | ||
CREATE_TEXT_QUESTION, |
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.
Let's change the mutation name.
|
||
import styles from './index.module.css'; | ||
|
||
const CREATE_TEXT_QUESTION = gql` |
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.
Let's change the mutation names.
<SelectInput | ||
name="label" | ||
label="Pillar and Sub pillar" | ||
value={formValue.label} | ||
error={fieldError?.label} | ||
onChange={setFieldValue} | ||
keySelector={pillarKeySelector} | ||
labelSelector={pillarLabelSelector} | ||
options={pillarsOptions} | ||
/> |
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 should make a separate component for this SelectInput. Also, let's make this a SearchSelectInput.
20134c1
to
5df9814
Compare
4846faa
to
4f70c3f
Compare
d61774d
to
8793067
Compare
8793067
to
d61774d
Compare
return ( | ||
<SelectInput | ||
name={name} | ||
label="Pillar and Sub pillar" | ||
value={value} | ||
error={error} | ||
onChange={onChange} | ||
keySelector={pillarKeySelector} | ||
labelSelector={pillarLabelSelector} | ||
options={pillarsOptions} | ||
/> | ||
); |
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.
please pass disable also
return ( | ||
<div className={_cs(styles.preview, className)}> | ||
<TextOutput | ||
value={label ?? 'Which Country needs the assistance quickest?'} | ||
description={hint ?? 'Choose One'} | ||
spacing="none" | ||
block | ||
/> | ||
<ListView | ||
className={styles.questionList} | ||
data={optionsListResponse?.private?.projectScope?.choiceCollection?.choices} | ||
keySelector={checkboxKeySelector} | ||
renderer={Checkbox} | ||
rendererParams={checkboxListRendererParams} | ||
filtered={false} | ||
errored={false} | ||
pending={false} | ||
/> | ||
</div> |
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.
Let's also handle case where choices are empty
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 this is okay. The list won't be displayed in case of empty data which is exactly what we want.
import { | ||
_cs, | ||
isNotDefined, | ||
noOp, | ||
} from '@togglecorp/fujs'; | ||
import { useMemo } from 'react'; |
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.
maintain order
<RadioInput | ||
className={styles.questionList} | ||
keySelector={choiceCollectionKeySelector} | ||
label="Options" | ||
labelSelector={choiceCollectionLabelSelector} | ||
name="options" | ||
onChange={noOp} | ||
options={optionsList as QuestionChoiceCollectionType[]} | ||
value={optionsListResponse?.private?.projectScope?.choiceCollection?.name} | ||
/> |
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.
Let's pass disable here too
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 should be readOnly as we're only previewing the options, not letting users choose from the options. @barshathakuri Let's add a 'readOnly' everywhere in preview.
onQuestionClick: React.Dispatch<React.SetStateAction<string | undefined>>; | ||
} | ||
|
||
function QuestionTypeItem(props: Props) { |
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.
Let's rename to QuestionTypeButton, pass disable from props
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.
Also, this should be inside components/questionPreviews
const pillarKeySelector = (data: Pillar) => data.id; | ||
const pillarLabelSelector = (data: Pillar) => data.label; |
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.
define these outside
name="choiceCollection" | ||
keySelector={choiceCollectionKeySelector} | ||
label="Options" | ||
labelSelector={choiceCollectionLabelSelector} | ||
onChange={setFieldValue} | ||
onSearchValueChange={setSearch} | ||
onOptionsChange={setChoiceCollectionOptions} | ||
searchOptions={searchOption} | ||
options={choiceCollectionOptions} | ||
onShowDropdownChange={setOpened} | ||
value={formValue.choiceCollection} | ||
/> |
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.
reuse created component
return ( | ||
<div className={_cs(styles.preview, className)}> | ||
<TextOutput | ||
value={label ?? 'Which Country needs the assistance quickest?'} |
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.
replace dummy text with "Title"
labelSelector={choiceCollectionLabelSelector} | ||
name="options" | ||
onChange={noOp} | ||
options={optionsList as QuestionChoiceCollectionType[]} |
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.
cast shouldn't be done
choiceCollections( | ||
filters: { | ||
questionnaire: {pk: $questionnaireId}, | ||
name: {iContains: $search } |
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 if we want to filter the choiceCollections, we should do it through label
instead of name
.
]); | ||
|
||
const { | ||
data: choiceCollectionsResponse, |
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.
Let's fetch loading
as well. We need to disable the SelectInput when the query is loading.
onShowDropdownChange={setOpened} | ||
value={value} | ||
label={label} | ||
error={error} |
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.
Let's include disabled props.
]); | ||
|
||
const { | ||
data: pillarsResponse, |
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.
Let's fetch loading
as well.
keySelector={pillarKeySelector} | ||
labelSelector={pillarLabelSelector} | ||
options={pillarsOptions} | ||
disabled={false} |
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.
The SelectInput is disabled when the query is loading. Let's include that as well.
options={optionsList} | ||
value={optionsListResponse?.private?.projectScope?.choiceCollection?.name} | ||
readOnly | ||
disabled={false} |
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.
Let's add disabled when the query is loading.
const [ | ||
triggerQuestionCreate, | ||
{ loading: createQuestionPending }, | ||
] = useMutation<CreateMultipleSelectionQuestionMutation, CreateTextQuestionMutationVariables>( |
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.
] = useMutation<CreateMultipleSelectionQuestionMutation, CreateTextQuestionMutationVariables>( | |
] = useMutation<CreateMultipleSelectionQuestionMutation, CreateMultipleSelectionQuestionMutationVariables>( |
PartialForm, | ||
} from '@togglecorp/toggle-form'; | ||
import { | ||
CreateTextQuestionMutationVariables, |
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.
CreateTextQuestionMutationVariables, | |
CreateMultipleSelectionQuestionMutationVariables, |
} from '@togglecorp/toggle-form'; | ||
|
||
import { | ||
CreateTextQuestionMutationVariables, |
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.
CreateTextQuestionMutationVariables, | |
CreateSingleSelectQuestionMutationVariables, |
{ loading: createQuestionPending }, | ||
] = useMutation< | ||
CreateSingleSelectionQuestionMutation, | ||
CreateTextQuestionMutationVariables |
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.
CreateTextQuestionMutationVariables | |
CreateSingleSelectionQuestionMutationVariables |