-
-
Notifications
You must be signed in to change notification settings - Fork 178
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-1058] Fix registration links #5089 #5090
base: main
Are you sure you want to change the base?
Conversation
46b33b2
to
1b172e8
Compare
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.
Cool!
@pauloamorimbr During review / edge case testing I made some suggested changes in a separate branch (https://github.com/kobotoolbox/kpi/tree/5089-fix-registration-links--additions):
- Checked what it looks like when an SSO provider is configured — I color-matched the SSO separator with this new links one (went with the new color)
- Fixed a layout issue on signup page on narrow screen width
- Also fixed an unrelated color issue that was introduced recently — changed 'required' asterisks back to light red
- Remove extra separator if the Django admin didn't configure Privacy Policy / Terms links at all
Compare: https://github.com/kobotoolbox/kpi/compare/5089-fix-registration-links..5089-fix-registration-links--additions — (you'd need to merge 'main' into '5089-fix-registration-links' first to see a clean diff)
+sso | |
---|---|
Other questions (not included above)
- 🎨 @tesster7 What do you think of these legal link colors/sizes/etc.? I wonder if we'd want them to match more closely with the Create Account / Forgot password or not. The contrast is still a little low imo but maybe want to avoid it being too busy.
sso-specific pages
- 🔧 @pauloamorimbr There are 2 more SSO-related templates (under kpi/kobo/apps/accounts/templates/socialaccount/ — login.html and signup.html) @tinok should we put the Terms / Privacy links here too, or is that not necessary? (the deep signup / login pages that you get if you're using SSO.)
sso-specific login page | e.g. with legal links |
---|---|
<div class="registration__legal"> | ||
{% if config.TERMS_OF_SERVICE_URL %} | ||
<a href="{{config.TERMS_OF_SERVICE_URL}}" target="_blank"> | ||
{% trans "Terms of Service" %} | ||
</a> | ||
{% endif %} | ||
{% if config.PRIVACY_POLICY_URL %} | ||
<a href="{{config.PRIVACY_POLICY_URL}}" target="_blank"> | ||
{% trans "Privacy Policy" %} | ||
</a> | ||
{% endif %} | ||
</div> |
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.
edit: incorporated in the review branch [xref 💬]
Suggest wrapping the div for registration__legal in an inclusive check {% if config.TERMS_OF_SERVICE_URL or config.PRIVACY_POLICY_URL %}
to remove this separator line if it's not needed:
Checklist
Description
Terms of Service and Privacy Policy links are now places inside the darker area to provide better visibility.
Notes
Some changes in the UI were made based on this discussion in Zulip.
Production:
This PR:
Related issues
Fixes #5089