-
Notifications
You must be signed in to change notification settings - Fork 4
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
Fix various onboarding issues #366
Conversation
✅ Deploy Preview for ami-web ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -12,7 +12,9 @@ export const convertToFormData = (fieldValues: DeploymentFieldValues) => { | |||
longitude: fieldValues.longitude, | |||
research_site_id: fieldValues.siteId, | |||
}).forEach(([key, value]) => { | |||
data.append(key, `${value}`) | |||
if (value && value !== 'undefined') { |
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.
@annavik I am curious how these variables are getting converted to the string "undefined". Any ideas?
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.
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.
If we want to make sure we will not pass the string "undefined" to server, we must make sure the value is not undefined before we try make a string out of it. Pretty much what you did, but the string comparison is not necessary. I would also make sure we will not ignore numeric values equal to 0 by making the check a bit more specific.
if (value && value !== 'undefined') { | |
if (value !== undefined) { |
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.
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.
If we want to make sure we will not pass the string "undefined" to server, we must make sure the value is not undefined before we try make a string out of it. Pretty much what you did, but the string comparison is not necessary. I would also make sure we will not ignore numeric values equal to 0 by making the check a bit more specific.
This is what I started with, but somehow the undefined value is converted to a string before this function is called. So value !== undefined
does not catch it :(
✅ Deploy Preview for ami-storybook ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -83,7 +83,7 @@ export const DeploymentDetailsInfo = ({ | |||
<FormSection title={translate(STRING.FIELD_LABEL_SOURCE_IMAGES)}> | |||
<FormRow> | |||
<InputValue | |||
label="Data source" | |||
label={translate(STRING.FIELD_LABEL_DATA_SOURCE)} |
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.
@annavik The other select fields for Site & Device have a placeholder "Pick a value", but Data Source does not. I can't see why, any ideas?
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.
Hi @jonbeu this should fix the issue you faced when creating a new deployment. The site, device & data source are no-longer required -- but also a default device & site are created for each project now. I am anxious to see some test images from the AMI-Mini! -Michael |
Users have been unable to create a new Deployment on their own. This is mostly because a Device, Research Site and Data Source need to be created first before creating a Deployment. Those related models are not actually required, but the form was requiring something to be selected. This PR makes several updates to address this issue.
Other changes: