Conversation
…ooter_menu from config['footer_menu'] into template
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR bumps the project version to 1.1.3, removes debugging prints from the admin update routine, and adds a configurable footer menu to the default template.
- Remove debug print statements in
update_set_org - Introduce a new
inject_footer_menucontext processor and renderfooter_menuin the base layout - Bump application version to 1.1.3 and update CHANGELOG
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| flowapp/views/admin.py | Removed debug print statements from the update_set_org function |
| flowapp/utils/app_factory.py | Added inject_footer_menu context processor for footer menu configuration |
| flowapp/templates/layouts/default.html | Updated layout to include a <footer> rendering dynamic footer_menu and version |
| flowapp/about.py | Bumped __version__ to 1.1.3 |
| README.md | Updated CHANGELOG entry for version 1.1.3 |
Comments suppressed due to low confidence (3)
flowapp/views/admin.py:674
- Consider using the application logger (e.g., logger.debug) instead of print statements for debug output so it can be properly managed and filtered in production.
data_records = model.query.filter(model.org_id == 0).all()
flowapp/views/admin.py:684
- Consider using the application logger for diagnostic messages instead of print statements to maintain consistent logging practices.
else:
flowapp/utils/app_factory.py:112
- It would be helpful to add a unit test to verify that the footer_menu context processor correctly injects the FOOTER_MENU configuration into the template context.
def inject_footer_menu():
| <div class="container"> | ||
| <div class="row justify-content-between align-items-center"> | ||
| <div class="col-md-auto mb-2 mb-md-0"> | ||
| <nav class="nav"> |
There was a problem hiding this comment.
Consider adding an aria-label (e.g., aria-label="Footer navigation") to the
element to improve accessibility for screen readers.
Suggested change
| <nav class="nav"> | |
| <nav class="nav" aria-label="Footer navigation"> |
|
|
||
|
|
||
| ## Change Log | ||
| - 1.1.3 - introduced configurable footer menu for links in bottom of the default template |
There was a problem hiding this comment.
[nitpick] Consider rephrasing to 'introduced configurable footer menu for links at the bottom of the default template' for improved readability.
Suggested change
| - 1.1.3 - introduced configurable footer menu for links in bottom of the default template | |
| - 1.1.3 - introduced configurable footer menu for links at the bottom of the default template |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.