-
Notifications
You must be signed in to change notification settings - Fork 33
fix: Only show language menu for more than one language #457
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
Conversation
Reviewer's Guide by SourceryThis pull request fixes an issue where the language menu was being displayed even when only one language was configured for the site. The fix ensures that the language menu is only rendered when there are multiple languages available. A test case has been added to verify this behavior. Sequence diagram for adding language menusequenceDiagram
participant Toolbar
participant cms_toolbars.add_language_menu
participant get_language_tuple
Toolbar->>cms_toolbars.add_language_menu: Call add_language_menu()
cms_toolbars.add_language_menu->>get_language_tuple: Get languages for site
alt len(languages) < 2
cms_toolbars.add_language_menu-->>Toolbar: Return (no menu added)
else len(languages) >= 2
cms_toolbars.add_language_menu->>Toolbar: Add language menu
end
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #457 +/- ##
==========================================
+ Coverage 90.54% 90.55% +0.01%
==========================================
Files 72 72
Lines 2729 2732 +3
Branches 321 322 +1
==========================================
+ Hits 2471 2474 +3
Misses 182 182
Partials 76 76 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 @fsbraun - I've reviewed your changes - here's some feedback:
Overall Comments:
- The test case added is good, but it would be better if it also asserted that the language menu would be present with multiple languages configured.
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.
Description
django CMS versioning "takes over" the language menu for pages. In accordance with the behavior of the django CMS core it should only show the language menu if there is more than one language configured.
Fixes one of django-cms/django-cms#8158
Related resources
Checklist
master
Slack to find a “pr review buddy” who is going to review my pull request.
Summary by Sourcery
Bug Fixes: