Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for an optional “white logo” asset so sponsor logos can be displayed appropriately on dark backgrounds (e.g., the PyPI footer).
Changes:
- Add
white_logoImageField toSponsorwith migration support. - Accept and persist
white_logovia sponsorship application and sponsor update forms. - Expose
white_logovia the logo placements API and display it in the Sponsorship admin view.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/sponsors/serializers.py | Extends logo placement serializer to include white_logo URL. |
| apps/sponsors/models/sponsors.py | Adds optional white_logo image field to Sponsor model. |
| apps/sponsors/migrations/0104_add_sponsor_white_logo.py | Schema migration adding the white_logo column. |
| apps/sponsors/forms.py | Adds white_logo upload field and saves it on sponsor creation/update. |
| apps/sponsors/api.py | Includes white_logo URL (or null) in logo placement API response. |
| apps/sponsors/admin.py | Adds admin display method for sponsor white logo thumbnail on Sponsorship admin. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 77dff453c6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
apps/users/templates/users/sponsor_info_update.html:250
- Same spacing issue as above: when
print_logois present, the “Currently” link and the help text are adjacent with no line break/block separation, which can make the UI hard to read. Add a<br/>before the helptext (or wrap “Currently” in its own block element).
<br/>Currently: <a href="{{ sponsor.print_logo.url }}">{{ sponsor.print_logo.name }}</a>
{% endif %}
{% if form.print_logo.help_text %}
<span class="helptext">{{ form.print_logo.help_text }}</span>
{% endif %}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <br/>Currently: <a href="{{ sponsor.web_logo.url }}">{{ sponsor.web_logo.name }}</a> | ||
| {% endif %} | ||
| {% if form.web_logo.help_text %} | ||
| <span class="helptext">{{ form.web_logo.help_text }}</span> | ||
| {% endif %} |
There was a problem hiding this comment.
Currently: is rendered inline via <br/>Currently: ... and then the help text <span class="helptext">…</span> follows without a line break or block wrapper. This will cause the help text to run together with the “Currently” link on the same line. Consider adding a <br/> before the helptext (or wrapping the “Currently” link in its own block element) for consistent spacing.
| <br/>Currently: <a href="{{ sponsor.white_logo.url }}">{{ sponsor.white_logo.name }}</a> | ||
| {% endif %} | ||
| {% if form.white_logo.help_text %} | ||
| <span class="helptext">{{ form.white_logo.help_text }}</span> | ||
| {% endif %} |
There was a problem hiding this comment.
Same spacing issue as web_logo: the “Currently” link and the help text are rendered back-to-back without a line break, so the help text will appear immediately after the link. Add a <br/> before the helptext (or wrap “Currently” in its own block element).
| <p class="form_field"> | ||
| <label>{{ form.landing_page_url.label }} <span class="error-message">{% if form.landing_page_url.errors %} | ||
| {{ form.landing_page_url.errors.as_text }}</span>{% endif %}</label> | ||
| <span | ||
| class="helptext">Landing page URL. The linked page may not contain any sales or marketing information.</span> | ||
| {% render_field form.landing_page_url %} | ||
| </p> |
There was a problem hiding this comment.
This template refactors several fields from being inside .inline_fields wrappers to standalone <p class="form_field"> blocks (e.g., landing page / Twitter / LinkedIn). That changes the layout behavior tied to the .inline_fields CSS rules in static/css/style.css:3485-3493 (inline-block 49% columns). If the layout change isn’t intended as part of “allow white logos”, consider reverting/splitting these markup changes or adjusting the grouping so the original 2-column layout is preserved.
| <p class="form_field"> | ||
| <label>{{ form.web_logo.label }} <span class="error-message">{% if form.web_logo.errors %}{{ form.web_logo.errors.as_text }}</span>{% endif %}</label> | ||
| {% render_field form.web_logo %} | ||
| {% if sponsor.web_logo %} | ||
| <p>Currently: <a href="{{ sponsor.web_logo.url }}">{{ sponsor.web_logo.name }}</a></p> | ||
| {% endif %} | ||
| {% if form.web_logo.help_text %} | ||
| <br/> | ||
| <span class="helptext">{{ form.web_logo.help_text }}</span> | ||
| {% endif %} | ||
| </p> | ||
| </div> | ||
| {% render_field form.web_logo %} | ||
| {% if sponsor.web_logo %} | ||
| <br/>Currently: <a href="{{ sponsor.web_logo.url }}">{{ sponsor.web_logo.name }}</a> | ||
| {% endif %} | ||
| {% if form.web_logo.help_text %} | ||
| <br/> | ||
| <span class="helptext">{{ form.web_logo.help_text }}</span> | ||
| {% endif %} | ||
| </p> |
There was a problem hiding this comment.
The .inline_fields wrapper around the logo upload fields was removed, which will change the 2-column layout controlled by static/css/style.css:3485-3493. With the new white_logo field, consider regrouping the logo inputs into one or more .inline_fields sections (e.g., web_logo + white_logo side-by-side, print_logo below) so the layout remains consistent with existing styling.
| <p class="form_field"> | ||
| <label>{{ form.landing_page_url.label }} <span class="error-message">{% if form.landing_page_url.errors %}{{ form.landing_page_url.errors.as_text }}</span>{% endif %}</label> | ||
| {% render_field form.landing_page_url %} | ||
| {% if form.landing_page_url.help_text %} | ||
| <br/> | ||
| <span class="helptext">{{ form.landing_page_url.help_text }}</span> | ||
| {% endif %} | ||
| </p> | ||
|
|
||
| <div> | ||
| <p class="form_field"> | ||
| <label>{{ form.twitter_handle.label }} <span class="error-message">{% if form.twitter_handle.errors %}{{ form.twitter_handle.errors.as_text }}</span>{% endif %}</label> | ||
| {% render_field form.twitter_handle %} | ||
| {% if form.twitter_handle.help_text %} | ||
| <br/> | ||
| <span class="helptext">{{ form.twitter_handle.help_text }}</span> | ||
| {% endif %} | ||
| </p> | ||
| </div> | ||
| <p class="form_field"> | ||
| <label>{{ form.twitter_handle.label }} <span class="error-message">{% if form.twitter_handle.errors %}{{ form.twitter_handle.errors.as_text }}</span>{% endif %}</label> | ||
| {% render_field form.twitter_handle %} | ||
| {% if form.twitter_handle.help_text %} | ||
| <br/> | ||
| <span class="helptext">{{ form.twitter_handle.help_text }}</span> | ||
| {% endif %} | ||
| </p> |
There was a problem hiding this comment.
This template also removes an .inline_fields grouping for the landing page / Twitter / LinkedIn fields. Since .inline_fields has specific 2-column styling (static/css/style.css:3485-3493), please confirm the layout change is intentional; otherwise, restore the wrapper or adjust the grouping to keep the intended column layout.
Description