-
Notifications
You must be signed in to change notification settings - Fork 9
CARDS-2679: Populate selectable area question with selectable zone options on variant selection #1960
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
base: dev
Are you sure you want to change the base?
Conversation
0ab1fa8 to
a2e97ac
Compare
This comment was marked as resolved.
This comment was marked as resolved.
modules/data-entry/src/main/frontend/src/questionnaireEditor/AnswerOptions.jsx
Outdated
Show resolved
Hide resolved
modules/data-entry/src/main/frontend/src/questionnaireEditor/ListInput.jsx
Show resolved
Hide resolved
modules/data-entry/src/main/frontend/src/questionnaireEditor/bodyParts.json
Outdated
Show resolved
Hide resolved
a2e97ac to
4b67215
Compare
|
@marta- Can you please post here what we decided to do on the last meeting? My memory is not clear |
...ata-entry/src/main/resources/SLING-INF/content/libs/cards/dataEntry/SelectableArea/Face.json
Outdated
Show resolved
Hide resolved
modules/data-model/forms/api/src/main/resources/SLING-INF/nodetypes/forms.cnd
Outdated
Show resolved
Hide resolved
modules/data-entry/src/main/frontend/src/questionnaireEditor/ListInput.jsx
Show resolved
Hide resolved
| if (optionsLoaded && fieldsReader?.variant && fieldsReader.variant.length > 0) { | ||
| let selectableZones = {}; //here we get the zones json from the JCR node corresponding to variant |
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.
Instead of pulling variant specifically, we look through the context and see if any of the stored fields also provide defaultOptions. If yes, we use that map to load generate the answer options. If there's more than one entry in the context that specifies defaultOptions, I think it's reasonable to only take into account the first one.
| <FormattedText variant="caption">{hint}</FormattedText> | ||
| }> | ||
| <Info color="primary" /> | ||
| <Info color="primary" sx={{mb: "-2px", ml: "2px"}}/> |
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 doesn't seem to have any effect. Instead, adding display: flex to the Typography style helps improve and control the alignment.
| //Children | ||
|
|
||
| // Container for the selectable zones names corresponding to the variant | ||
| + defaultOptions (nt:unstructured) |
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.
Add new line at the end of the file.
| final Function<Node, JsonValue> serializeNode) | ||
| { | ||
| try { | ||
| if (child.getName().equals("defaultOptions")) { |
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.
Even though child is expected to have a name, flipping the two is always safer:
| if (child.getName().equals("defaultOptions")) { | |
| if ("defaultOptions".equals(child.getName())) { |
| "@path": path + "/AnswerOption" + stringToHash(key) | ||
| })); | ||
|
|
||
| setOptions(variantOptions); |
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.
Bug: every time a selectable area question is opened for editing, the options are overwritten with the ones defined by the selected variant.
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.
Addressed in b8ceeea - although I'm not very happy with the method (having a state variable that is set to true when options are generated from defaultOptions, and to false in all places that update options from user input. A more elegant solution is welcome.
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.
Bug: now once user customes any variat selection it's not possible to change variant and prepopulate another set.
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.
That is intentional so the user wouldn't lose their work.
However, it sounds like the right way to approach this is to enable the user to choose what they want to do with the suggested answer options. When different suggested options are available in the context, display a dialog that shows existing options (if any) and suggested options, and allow the user to merge the suggested into the existing, replace the existing with the suggested, or ignore the suggestions.
| .map(([key, value]) => ({ | ||
| label: value, | ||
| value: key, | ||
| noneOfTheAbove : 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.
noneOfTheAbove: false is unnecessary.
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.
Removed in b8ceeea .
| label: value, | ||
| value: key, |
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.
Not a big deal, but label = value and value = key is a bit odd. Better names for the arguments would help.
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.
Changed [key, value] to [key, label] in b8ceeea .
| // Pre-populate selectableQuestion answer options with selectable zones according to the selected variant if any selected | ||
| useEffect(() => { | ||
| if (fieldsReader?.variant?.[0]?.defaultOptions) { | ||
| let variantOptions = Object.entries(fieldsReader?.variant?.[0]?.defaultOptions) | ||
| .filter(([key]) => !key.startsWith("@") && !key.startsWith("jcr:")) | ||
| .map(([key, value]) => ({ | ||
| label: value, | ||
| value: key, | ||
| noneOfTheAbove : false, | ||
| "@path": path + "/AnswerOption" + stringToHash(key) | ||
| })); | ||
|
|
||
| setOptions(variantOptions); | ||
| } | ||
| }, | ||
| [fieldsReader.variant]); | ||
|
|
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 code still references variant specifically, and the comments mention selectable zones. Instead, it should be agnostic of the question type. Any structured property that defines defaultOptions should be able to populate the answer 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.
Addressed in commit b8ceeea - only lightly tested.
|
Usability issue: now that the |
…tions on variant selection
…tions on variant selection Better variable name
…tions on variant selection Proper resource management
…tions on variant selection Addressed codereview comments
…tions on variant selection * Made the code that extracts defaultOptions from variant nodes more generic * Bug fix: prevented the variant's defaultOptions from always overwriting user options * Minor polishing of the generated options
b8ceeea to
5db6aae
Compare
CARDS-2679