-
-
Notifications
You must be signed in to change notification settings - Fork 181
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
[TASK-623] Allow users to select which form questions to display in the single processing side bar #4889
[TASK-623] Allow users to select which form questions to display in the single processing side bar #4889
Conversation
jsapp/js/components/processing/sidebar/sidebarDisplaySettings.tsx
Outdated
Show resolved
Hide resolved
jsapp/js/components/processing/sidebar/sidebarDisplaySettings.tsx
Outdated
Show resolved
Hide resolved
jsapp/js/components/processing/sidebar/sidebarDisplaySettings.tsx
Outdated
Show resolved
Hide resolved
jsapp/js/components/processing/sidebar/sidebarDisplaySettings.tsx
Outdated
Show resolved
Hide resolved
jsapp/js/components/processing/sidebar/sidebarDisplaySettings.tsx
Outdated
Show resolved
Hide resolved
jsapp/js/components/processing/sidebar/sidebarDisplaySettings.tsx
Outdated
Show resolved
Hide resolved
jsapp/js/components/processing/sidebar/sidebarDisplaySettings.tsx
Outdated
Show resolved
Hide resolved
jsapp/js/components/processing/sidebar/sidebarDisplaySettings.tsx
Outdated
Show resolved
Hide resolved
jsapp/js/components/processing/sidebar/sidebarDisplaySettings.tsx
Outdated
Show resolved
Hide resolved
jsapp/js/components/processing/sidebar/sidebarDisplaySettings.tsx
Outdated
Show resolved
Hide resolved
jsapp/js/components/processing/sidebar/sidebarDisplaySettings.tsx
Outdated
Show resolved
Hide resolved
jsapp/js/components/processing/sidebar/sidebarDisplaySettings.tsx
Outdated
Show resolved
Hide resolved
@@ -158,6 +266,7 @@ export default function SidebarDisplaySettings() { | |||
color='light-blue' | |||
size='m' | |||
onClick={() => { | |||
applyFieldsSelection(); | |||
store.setDisplays(activeTab, selectedDisplays); | |||
setIsModalOpen(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.
I've found a small bug 🐛:
- I open settings modal
- I have all questions selected
- I unselect some questions
- I've decided I don't want to change it so I close modal (e.g. with ESC)
- I open settings modal again
- I have some questions selected (it should be all questions selected, i.e. it should reflect what we have in store)
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 a bug with the modal in general. Maybe we never caught it before but the same steps above can be taken for the regular toggle switches (the displays). And it's due to this: https://github.com/kobotoolbox/kpi/pull/4889/files/be15a3938a5956f36531d3e2325e0774cecfe635#diff-4e96bc318c90b92e52221bc45959e508183d0babeb08d081a876bcccdcc3d542L146-L149
After some thought my idea is to treat clicking off or closing the modal as a local state reset; i.e., run a check of all the displays/fields versus what is turned off/hidden in the store on closing. What do you think?
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 a fix would be to apply setSelectedFields(getInitialFields());
inside the onRequestClose
function. This way after closing and reopening, the state would be fresh. Alternatively we could do it in the same function that is opening the modal (doing setIsModalOpen(true)
). So either "cleanup afterwards" or "always start fresh" approach :D After doing either of these, please check if one of the places that resets its already is not needed (e.g. inside resetFieldsSelection
, or that effect when tab is changed)
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 these are still needed in resetFieldsSelection
as resetting is not the same as closing the modal--on reset, we need to tell the store to clear the hidden fields as well as reset them visually with setSelectedFields(getInitialFields());
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 a bug with the modal in general. Maybe we never caught it before but the same steps above can be taken for the regular toggle switches (the displays). And it's due to this: https://github.com/kobotoolbox/kpi/pull/4889/files/be15a3938a5956f36531d3e2325e0774cecfe635#diff-4e96bc318c90b92e52221bc45959e508183d0babeb08d081a876bcccdcc3d542L146-L149
I cleared this up by adding setSelectedDisplays(store.getDisplays(activeTab))
into onRequestClose
as well
jsapp/js/components/processing/sidebar/sidebarDisplaySettings.tsx
Outdated
Show resolved
Hide resolved
Conflicts: jsapp/js/components/processing/sidebar/sidebarDisplaySettings.tsx
Conflicts: jsapp/js/components/processing/sidebar/sidebarDisplaySettings.tsx
Description
Adds a checkbox that allows choosing which questions to display in the single processing sidebar. Checkbox only appears if "Submission Data" is toggled on.
Notes
Because of the way
SubmissionDataList
hides questions, the displayed checkbox and the information sent to the store had to be inverted i.e., we send a list of 'hidden' questions rather the list of checked boxes.Related issues
NLP addons: https://www.notion.so/kobotoolbox/NLP-Qualitative-analysis-improvements-11b843e113dd478ca33010eefa2814b1?p=e1a31c80b74f4d569bf2505aa68fb9e2&pm=c