-
-
Notifications
You must be signed in to change notification settings - Fork 63
Project specific ODK form #2492
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
Conversation
src/mapper/static/config.json
Outdated
@@ -3,7 +3,6 @@ | |||
"logoText": "Humanitarian OpenStreetMap Team", | |||
"cssFile": "https://cdn.jsdelivr.net/npm/@hotosm/[email protected]/dist/style.css", | |||
"cssFileWebformsOverride": "", | |||
"enableWebforms": 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 values are here as examples, plus this is the default file if no config in S3.
I think we will need
enableWebforms: true
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.
Since we are using the value of use_odk_collect from the project API, why do we need this?
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 there is a misunderstanding on the use of both vars:
- enableWebforms is per instance of fieldtm (its a feature flag to enable web forms functionality, else the instance uses ODK Collect for every project (ignore use_odk_collect)
- The use_odk_collect param is per project and is an override. Its anticipated that most uses use web forms, but its good to have an override if necessary (web forms has some missing form functionality)
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 I finally got your point:
You mean that if enableWebforms is false, though setting the use_odk_collect to false during project creation, we don't show webforms because we have disabled the feature of webforms in that particular instance.
And if enableWebforms is true, the user can override the behaviour of using webforms by setting the user_odk_collect to true, and the user can use ODK Collect instead of webforms.
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, there seems to be a bug with this condition:
if (project.use_odk_collect) {
commonStore.setUseOdkCollectOverride(project.use_odk_collect);
}
What's happening here is, the initial state of useOdkCollectOverride
is false, when a project with use_odk_collect set to true is opened, then the useOdkCollectOverride is set to true. Now if a user then opens a project where use_odk_collect set to false, the state of useOdkCollectOverride is not updated to false, due to which only mapping through ODK app is set.
@@ -48,35 +48,33 @@ | |||
let latestEventTime: string = $state(''); | |||
let isGeometryCreationLoading: boolean = $state(false); | |||
|
|||
const projectId = $derived(data.projectId); | |||
const project = $derived(data.project); |
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.
data doesn't need derived I don't think, but I think the recommended Svelte way is prop destructuring
const { project } = data
@@ -48,35 +48,33 @@ | |||
let latestEventTime: string = $state(''); | |||
let isGeometryCreationLoading: boolean = $state(false); | |||
|
|||
const projectId = $derived(data.projectId); |
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 100% on this, but I think all reactive svelte vars should use let
and not const
Forgot to switch the branch. This PR is also related to:
|
What type of PR is this? (check all applicable)
Related Issue
Describe this PR
Screenshots