feat: GH-999 let CMS admin change header logo - #1083
Conversation
There was a problem hiding this comment.
Before 53cfef6, I asked AI to review how to improve clarity:
AI Suggestions
1.
header_logo.html— > Name the three sourcesThe
@varand the three > branches don’t say where each > source comes from. A short > comment per branch helps:
- After the load/var block, > add a single line describing > the three cases, e.g.
{# Logo source: plugin_logo > (CMS Picture), settings.LOGO > (legacy), or settings.> PORTAL_LOGO. #}- Or add a one-line comment > above each of the three > blocks, e.g.
{# 1. From CMS static > placeholder "header-logo" > (Picture plugin). #}
{# 2. From settings.LOGO > (legacy tuple). #}
{# 3. From settings.> PORTAL_LOGO (default). #}That makes it obvious why > there are three branches and > what each represents.
2.
header_logo.html— > Document expected shape of >plugin_logoThe first and third branches > use the same
logo.*keys. To > make the contract explicit for > someone editing the tag or the > template, add a short comment > near the top, e.g.:
{# plugin_logo (when set) > must provide: link_href, > link_target, img_class, > img_file_src, is_remote, > img_crossorigin, img_alt_text > (same shape as PORTAL_LOGO). #}>Then it’s clear that >
plugin_logois “a > PORTAL_LOGO-shaped dict.”
3.
header_logo_tags.py> — Document the contract and > simplify alt
In the docstring, state that > the returned dict is > PORTAL_LOGO-shaped (or > “same keys as header_logo.html > expects for the plugin_logo / > PORTAL_LOGO branch”). That > ties the tag to the template > without reading both.
Build
altin two steps so > the fallback chain is easier > to read:
alt_from_attrs = (getattr> (instance, 'attributes', > None) or {}).get('alt')alt = alt_from_attrs or > (getattr(picture, > 'default_alt_text', None) if > picture else None) or ''Same behavior, but the > intent (“attribute alt, then > picture default, then > empty”) is obvious.
Optionally add a one-line > comment above the
return > { ... }: e.g. “Same keys as > header_logo.html expects > (PORTAL_LOGO shape).”
4.
header_logo_tags.py> — Clarify “instance”
- In the docstring, spell out > that
instanceis the > djangocms_picture plugin > instance (the one that has >picture,attributes, >link_target, and provides >img_srcin context). That > way “instance” isn’t ambiguous.
5.
picture.html— > Optional one-line “why”The comment already says “pass > plugin_logo and use shared > header_logo.html.” If you want > the reason in one line, you > could add:
{# Single markup in > header_logo.html; we only > supply plugin_logo from the > Picture plugin. #}
Summary
- Templates: Name the > three logo sources (plugin / > LOGO / PORTAL_LOGO) and > document the expected >
plugin_logoshape (same as > PORTAL_LOGO).- Tag: Docstring = “builds > a PORTAL_LOGO-shaped dict from > the Picture plugin instance”; > simplify alt with one or two > intermediate variables; > optionally a one-line comment > above the returned dict.
That should make the flow (Picture → tag → plugin_logo → header_logo.html’s first branch) and the data contract clear for the next reader.
|
Update: Since 053fdd7, placeholder only accepts "Header logo" (Picture) plugin. |
Changed `header-…` static placeholder to accept more than just a logo, and render intelligently. WARNING: Introduces magic…
- Enhanced the `_get_header_content_parsed` function to clarify the handling of logo and remaining plugins. - Updated comments for better understanding of the logic flow. - Fixed a typo in the variable name `is_picture_plugin`. - Adjusted return statements to ensure consistent output structure.
Fix misc. bugs in #1083. | default logo | custom logo | | - | - | | <img width="1280" height="400" alt="edit header - default image" src="https://github.com/user-attachments/assets/ef119dcc-e49a-460e-9853-06ef9a1521ee" /> | <img width="1280" height="400" alt="edit header - custom logo" src="https://github.com/user-attachments/assets/1759ecf7-458d-4ca2-bc26-1f3957973a54" /> | --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Wesley B <wesleyboar@users.noreply.github.com>
## Overview (1) Adds **Header logo** as a thin Picture plugin on the `header-content` static placeholder so superusers can change the portal logo in CMS. (2) Empty placeholder still uses settings. (3) Core-Portal `/cms/header/logo/markup/` returns the same published plugin output or settings fallback. ## Related - improves #1083 - supports #999 ## Changes - **added** `HeaderLogoPlugin` - **added** logo-only render helper and template tag (Core-Portal markup URL) - **updated** header logo slot and Core-Portal markup template - **renamed** settings fallback template - **removed** custom header logo Picture/markup cruft - **updated** GH-999 plan doc ## Testing 1. `make start` (or existing stack). 2. Empty `header-content`: settings logo on site nav and on http://localhost:8000/cms/header/logo/markup/ 3. Superuser: add **TACC Header → Header logo**, publish; verify Picture output (link, srcset, etc. as configured) and default `#header-logo` when Attributes have no `id`; placeholder editable in `?edit` 4. Core-Portal (or fetch markup URL with CMS session): published plugin HTML matches site logo for that user 5. Non-superuser cannot edit static placeholders; `footer-content` unchanged ## UI https://github.com/user-attachments/assets/167d9579-13ca-4c7d-a5d2-e0c0074235ac https://github.com/user-attachments/assets/c609d7cf-ef54-4cea-92fd-57a38d7e5ac4 ## Notes - Custom Core-CMS overrides of `header_logo_settings.html` must switch to `header_logo_via_settings.html`. - When `header-content` holds more than a logo (PR 2), site uses full placeholder; markup URL stays logo-only via `render.py`. --------- Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Wesley B <62723358+wesleyboar@users.noreply.github.com>
## Overview Pre-fills the Header logo plugin admin form when adding a new instance so editors see home link, height, original-size cropping, and margin defaults (PR #1083 to-do). ## Related - supports #1083 - supports #999 ## Changes - **added** `HeaderLogoForm` with add-only defaults - **updated** `HeaderLogoPlugin` to use the form - **updated** GH-999 plan follow-up note (settings / Core-Styles parity deferred) ## Testing 1. Delete Header logo from `header-content` if present. 2. Add **TACC Header → Header logo**; confirm form shows home page link, height 50, use original image, Attributes class `mr-5`. 3. Save, publish; confirm logo renders with link and expected markup. 4. Edit existing plugin; confirm defaults are not re-applied. ## UI https://github.com/user-attachments/assets/1679b110-7d9c-4dc4-88f6-aca81a74d52c https://github.com/user-attachments/assets/37265f24-6808-44cb-b8f0-ff1f4762a67d
66c7905 to
15fc05c
Compare
## Overview Header logo plugin and settings fallback match Core-Portal link/img classes. Portal logo picture template puts `portal-logo` on the image (even if a link is set). ## Related - supports #1083 - supports #999 - requires TACC/Core-Styles#648 - requires TACC/Core-Styles#649 ## Changes - **updated** header logo plugin render and settings fallback markup - **added** Portal logo picture template and `picture_attributes_img` on default Picture - **updated** Header logo form defaults (Portal template when Core Portal) - **updated** `@tacc/core-styles` dev dep (temporary pin) ## Testing 1. Empty `header-content`: settings logo uses `navbar-brand` on `<a>`, `portal-logo` on `<img>`. 2. With `PORTAL_IS_TACC_CORE_PORTAL`: add Header logo defaults to Portal logo; published output has `portal-logo` on `<img>` and `navbar-brand` on the link. 3. Core-Portal `/cms/header/logo/markup/` matches site logo for plugin vs settings paths. ## UI https://github.com/user-attachments/assets/4a186b0b-3625-4314-9154-2d6de6ffadc8 https://github.com/user-attachments/assets/dc629e0e-b4ec-4955-94a7-7c0b1cf91f84 https://github.com/user-attachments/assets/ec99e5e5-5d68-4db7-8e25-21ced4609bdf https://github.com/user-attachments/assets/39368ab6-8995-4aec-83c7-ccf8b2d31962 ## Notes Bumped `@tacc/core-styles` to a published version after Core-Styles PRs merge.
Set Attributes alt to "Project logo" for new Header logo plugins.
Exclude HeaderLogoPlugin from default placeholders; allow only in header-content CMS_PLACEHOLDER_CONF.
Updated formatting and structure of the Known Limitations section in the editable header plan documentation.
…m:TACC/Core-CMS into feat/GH-999-let-cms-admin-edit-header
) ## Overview Content editor groups should not be able to change sitewide static placeholders (footer, future header slots). This removes that permission when group setup runs, so existing sites drop it on the next deploy that runs the helper. ## Related - Relates-to #1171 - Split from #1083 ## Changes - **added** `del_perm` helper (mirror of `add_perm`) - **updated** `let_view_page_and_structure` to revoke `Can change static placeholder` instead of granting it ## Testing 1. On a dev DB, grant a content group `Can change static placeholder` (or use a site that still has it). 2. Run the management command path that calls `let_view_page_and_structure` for that group (same as existing group setup). 3. Confirm the group no longer has `Can change static placeholder` in Django admin. 4. Confirm superusers can still edit static placeholders. ## UI No UI change. ## Notes Includes a TODO to delete the revoke block after all sites have deployed once. Open question in code: whether “Sitewide Content Manager” should retain this permission.
## Overview The footer already uses the `footer-content` static placeholder; this adds a display name so Structure mode shows “Footer content” instead of the raw code. ## Related - Split from #1083 ## Changes - **updated** `CMS_PLACEHOLDER_CONF` with a `footer-content` entry and translated name only ## Testing 1. `make start`, log in as a user who can use Structure mode. 2. Open any page, enter Structure mode. 3. Confirm the footer static placeholder is labeled “Footer content”. ## UI Structure sidebar label only; no front-end markup change.
wesleyboar
left a comment
There was a problem hiding this comment.
More notes for reviewers…
Warning
Overcomplicated. Simplifying. See #1189.
There was a problem hiding this comment.
Set default configuration of a Picture-as-Header-logo.
There was a problem hiding this comment.
Allow us to capture logo output so we can:
- see if logo is present in placeholder
- render logo only if it is available
| None: { | ||
| 'excluded_plugins': ['HeaderLogoPlugin'], | ||
| }, | ||
| HEADER_LOGO_PLACEHOLDER_NAME: { | ||
| 'name': _('Header logo'), | ||
| 'plugins': ['HeaderLogoPlugin'], | ||
| 'excluded_plugins': [], | ||
| 'limits': { | ||
| 'global': 1, | ||
| 'HeaderLogoPlugin': 1, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Name the placeholder and limit its content to a header logo.
There was a problem hiding this comment.
Create header logo plugin (extends Picture) so we can control:
- requirements (via this file)
- defaults (via
forms.py)
## Overview Stacks on #1083. Drops the extra ContentRenderer path and duplicate settings template; `header.html` owns the static placeholder, `header_logo.html` stays settings markup as on `main`. ## Related - requires #1083 - footer Structure label: #1188 (merged to `main`) - static-placeholder perm: #1187 (merged to `main`) ## Changes - **deleted** `render.py`, `constants.py`, `header_tags`, and `header_logo_via_settings.html` - **updated** `header.html`: `{% static_placeholder "header-logo" or %}` → `header_logo.html` - **updated** `CMS_PLACEHOLDER_CONF` for `header-logo` (keeps `footer-content` with #1188) - **updated** `docs/gh-999-editable-header-plan.md` (minimal deltas) ## Testing Same as #1083 steps 1–4 after this branch is merged into `feat/GH-999-let-cms-admin-edit-header`. ## UI No new UI; structure only.
Matches settings fallback anchor id for tests and cross-repo selectors.
There was a problem hiding this comment.
The keystone code is in this file.
Overview
Important
Reviewers!
What to name this plugin?
djangocms_tacc_header_logo(like our others)
If independent repo, yes.
taccsite_header_logo/(like
/taccsite_cmsand/taccsite_card)If placed at root, yes. (The
/taccsite_cardis new; I can still move it.)taccsite_cms/cms_plugins/(like
designsafe/apps/cms_plugins)But stuffing all the CMS plugins into one app seems like a step backward.
Where to put this plugin?
/…(root)
/taccsite_cms/apps/…(like
designsafe/apps)/taccsite_cms/plugins/…/taccsite_cms/cms_plugins/…(like
designsafe/apps)But stuffing all the CMS plugins into one app seems like a step backward.
github:TACC/Core-CMS-Plugin-Header-Logo(like our others)
But header logo is coupled to Core-CMS and Core-Styles.
Related
closes Let CMS Admin Edit Header #999
review request: WC-470
includes…
Changes
adds static placeholder
adds plugin
updates header template
adds default values for header logo
PORTAL_IS_TACC_CORE_PORTALTesting
Tip
Available to test on —
(new deploy might remove the feature)
— and you may deploy on Core-Portal, but delete plugin instance when you're done.
UI
UI, a little outdated
Now it's "Header logo" not "Header-Content", and I restored
id="header-logo"(only used by Portal's copy of header wrapper markup; keep or consistency).Add.a.logo.mov
A "Plugin Importer" visible in video is not new and not ours. Code to remove it is excessive.
the.Portal.logo.template.adds.portal-logo.class.mov
add.class.to.Picture.navbar-brand.is.always.present.mov
add.other.attribtues.to.Picture.still.works.and.migrates.to.container.mov
Only.header.content.allows.header.logo.mov