-
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
Fix: Login button label is missing #277
Fix: Login button label is missing #277
Conversation
Many pages share a common button template, so we need to compute it from context processors.
Reviewer's Guide by SourceryThis pull request fixes the missing login button label by dynamically generating it from context processors based on the Sequence diagram for login button label generationsequenceDiagram
participant User
participant Request
participant ContextProcessors
participant Settings
participant Phrases
User->>Request: Accesses a page
Request->>ContextProcessors: Calls context processors
ContextProcessors->>Settings: Retrieves site configuration
Settings-->>ContextProcessors: Returns site configuration
ContextProcessors->>Phrases: Retrieves login button label based on configuration
Phrases-->>ContextProcessors: Returns login button label
ContextProcessors-->>Request: Adds login button label to context
Request-->>User: Renders page with login button label
Updated class diagram for context processorsclassDiagram
class ContextProcessors {
+orga_events(request)
+collect_signal(signal, kwargs)
}
class Settings {
+CONFIG
+BASE_PATH
}
class Phrases {
+CALL_FOR_SPEAKER_LOGIN_BTN_LABELS
}
ContextProcessors -- Settings : Uses
ContextProcessors -- Phrases : Uses
File-Level Changes
Possibly 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:
- It looks like you're duplicating the login button label logic in
orga_events
andget_context_data
; can you consolidate this?
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 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.
Many pages share a common button template, so we need to compute it in context processors.
Fixes part of #276
This PR fixes the button label first.
For the top-right "login" link, I haven't figured out how it should be yet.
How has this been tested?
Checklist