-
Notifications
You must be signed in to change notification settings - Fork 66
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
Disable the subheader login link when there is already login button #280
Disable the subheader login link when there is already login button #280
Conversation
Reviewer's Guide by SourceryThis pull request introduces a mechanism to disable the subheader login link on specific pages, such as the event login page, to avoid user confusion when there is already a primary login button. A new context variable, No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @hongquan - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a setting to control the subheader link instead of a context variable.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@sourcery-ai |
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.
LGTM
Fixes #276
The issue is that, in the /talk/<event>/login/ page, we have a login button and a top-right login link, which lead to different locations (and hence, different behavior).
The top-right link is global and seems to serve different purpose (for upstream pretalx setup), I don't know how to change its location in a safe way (to not break other pages and to keep it easy to maintain). So I choose to hide it in the /talk/<event>/login/. This page already have a prominent login button which we want users to focus in, having more link will confuse users.
To do this, I introduce a context variable
subheader_login_link_disabled
, to be passed to templates and let the link show / hide according to this variable.I name it
subheader
because calling ittop-right
will confuse the Arabic UI user. In the future, we may support right-to-left languages and when displaying in those languages, the layout will be switched, the login link will be rendered on the left side.How has this been tested?
Checklist
Summary by Sourcery
This pull request disables the subheader login link on the event login page to avoid user confusion caused by having multiple login links. It introduces a context variable to control the visibility of the subheader login link.
Bug Fixes:
Enhancements:
subheader_login_link_disabled
context variable to control the visibility of the subheader login link in templates.