-
Notifications
You must be signed in to change notification settings - Fork 17
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
SBI Pre-launch Adjustments #343
Conversation
if sample_output['sample_site'] == "Cheek": | ||
# Format date and time to be JavaScript-friendly | ||
if sample_output['barcode_meta'][ | ||
'sample_site_last_washed_date' |
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'm unsure whether these keys are guaranteed to exist as the private API can return an empty dict? If they are not assured to exist then they could they be accessed through get
?
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.
All three keys should exist for any cheek sample, and their absence would indicate an unexpected state. However, using get
doesn't hurt anything and mitigates the minimal potential for an error.
@@ -203,6 +203,11 @@ | |||
{{ _('Thank you for logging your sample information. It looks like you haven\'t updated your profile recently. Please review') }} <a href="/accounts/{{account_id}}/sources/{{source_id}}/take_survey?survey_template_id={{prompt_survey_id}}">{{ _('your survey responses') }}</a> {{ _('to ensure they\'re as current and complete as possible.') }} | |||
</div> | |||
{% endif %} | |||
{% if prompt_skin_app %} | |||
<div class="alert alert-primary alert-nutrition mt-4" role="alert"> |
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.
Minor but this isn't nutrition?
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.
Adjusted, and adjusted the other prompt from which this was copied.
@@ -203,6 +203,11 @@ | |||
{{ _('Thank you for logging your sample information. It looks like you haven\'t updated your profile recently. Please review') }} <a href="/accounts/{{account_id}}/sources/{{source_id}}/take_survey?survey_template_id={{prompt_survey_id}}">{{ _('your survey responses') }}</a> {{ _('to ensure they\'re as current and complete as possible.') }} | |||
</div> | |||
{% endif %} | |||
{% if prompt_skin_app %} | |||
<div class="alert alert-primary alert-nutrition mt-4" role="alert"> | |||
{{ _('You now have access to the Skin Scoring App survey. Please return to the') }} <a href="/accounts/{{account_id}}/sources/{{source_id}}/take_survey?survey_template_id={{prompt_survey_id}}">{{ _('My Profile tab') }}</a> {{ _('to complete it.') }} |
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.
Why prompt rather than redirect?
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.
Accessing the Skin Scoring App isn't a simple redirect to an external site. It requires the user to open a pop-up on our side with instructions, hit a button to be allocated credentials, then hit a button to open the external site. As such, prompting the user to access it feels cleaner from a UX perspective than redirecting them to the My Profile tab with the pop-up already opened.
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.
Thanks. To check, does the pop up behave ok on mobile?
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.
Yes, the formatting is responsive and I've tested it using both Firefox and Chrome's developer-mode settings for a handful of different screen sizes.
@wasade Ready for re-review, thanks! |
This pull request addresses two issues: