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.
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.
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.
| </div> | ||
| {% endif %} | ||
| {% if prompt_skin_app %} | ||
| <div class="alert alert-primary alert-nutrition mt-4" role="alert"> |
There was a problem hiding this comment.
Adjusted, and adjusted the other prompt from which this was copied.
| {% 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.
Why prompt rather than redirect?
There was a problem hiding this comment.
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.
Thanks. To check, does the pop up behave ok on mobile?
There was a problem hiding this comment.
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! |
2 participants
This pull request addresses two issues: