align logo with content, replace language links with dropdown, and fix header alignment #1476
Conversation
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
…n, remove logo border radius, and add login alignment helper
Contributor
Reviewer's GuideWraps event and organizer logos in the main container for visual alignment, replaces inline language switcher links with accessible Sequence diagram for locale change via new select-based formsequenceDiagram
actor User
participant Browser
participant LocaleSetView
User->>Browser: Load_event_or_organizer_page
Browser->>User: Render_language_select_form_with_hidden_next
User->>Browser: Select_new_locale_option
Browser->>Browser: onchange_submit_language_form
Browser->>LocaleSetView: GET presale_locale_set
Note right of Browser: Query params: locale, next
LocaleSetView-->>Browser: 302_redirect_to_next_url
Browser->>Browser: Load_next_url_with_selected_locale
Browser-->>User: Display_page_with_updated_language
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The locale switcher form markup is now duplicated in several templates with only minor variations (IDs); consider extracting this into a shared fragment/partial to reduce repetition and keep behavior consistent.
- The new
.login-hdr--alignedclass relies on!importantto resetmargin-right; it would be more maintainable to adjust the underlying header layout or specificity so this override is not needed. - Using
onchange="this.form.submit()"on the<select>ties behavior to inline JS; consider moving this to a small unobtrusive script (and optionally adding a submit button as a graceful fallback) to keep templates focused on structure and accessibility.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The locale switcher form markup is now duplicated in several templates with only minor variations (IDs); consider extracting this into a shared fragment/partial to reduce repetition and keep behavior consistent.
- The new `.login-hdr--aligned` class relies on `!important` to reset `margin-right`; it would be more maintainable to adjust the underlying header layout or specificity so this override is not needed.
- Using `onchange="this.form.submit()"` on the `<select>` ties behavior to inline JS; consider moving this to a small unobtrusive script (and optionally adding a submit button as a graceful fallback) to keep templates focused on structure and accessibility.
## Individual Comments
### Comment 1
<location> `app/eventyay/presale/templates/pretixpresale/event/base.html:56` </location>
<code_context>
+ <option value="{{ l.code }}" {% if l.code == request.LANGUAGE_CODE %}selected{% endif %}>{{ l.name_local }}</option>
+ {% endfor %}
+ </select>
+ <input type="hidden" name="next" value="{{ request.path }}{% if request.META.QUERY_STRING %}?{{ request.META.QUERY_STRING|urlencode }}{% endif %}">
+ </form>
+ {% endif %}
</code_context>
<issue_to_address>
**suggestion:** Consider deduplicating the locale switcher markup and URL-building logic.
The language switcher form and `next` parameter construction are now repeated in multiple templates (`event/base.html` twice and `organizers/base.html` twice). Moving this into a shared include/fragment or a helper for generating `next` would reduce duplication and keep behavior consistent across templates.
Suggested implementation:
```
{% include "pretixpresale/partials/locale_switcher.html" %}
```
To fully implement the deduplication and keep behavior consistent:
1. Create a new template fragment at `app/eventyay/presale/templates/pretixpresale/partials/locale_switcher.html` with the full, DRY implementation:
```django
{% if event.settings.locales|length > 1 %}
<form action="{% url 'presale:locale.set' %}" method="get" class="locales" aria-label="{% trans "select language" %}">
<label for="locale-select" class="sr-only">{% trans "Language" %}</label>
<select id="locale-select" name="locale" onchange="this.form.submit()" class="form-control input-sm">
{% for l in languages %}
<option value="{{ l.code }}" {% if l.code == request.LANGUAGE_CODE %}selected{% endif %}>{{ l.name_local }}</option>
{% endfor %}
</select>
<input type="hidden" name="next" value="{{ request.path }}{% if request.META.QUERY_STRING %}?{{ request.META.QUERY_STRING|urlencode }}{% endif %}">
</form>
{% endif %}
```
2. Replace the other three duplicated locale-switcher blocks in `event/base.html` and `organizers/base.html` with `{% include "pretixpresale/partials/locale_switcher.html" %}` in the same way as above.
3. If different contexts use different `id` values for the `<select>` (e.g., multiple switchers on one page), either:
- Parameterize the `id` via `{% include ... with select_id="locale-select-header" %}`, or
- Remove the hardcoded `id` if it's not relied on by JavaScript or labels elsewhere.
</issue_to_address>
### Comment 2
<location> `app/eventyay/presale/templates/pretixpresale/organizers/base.html:22-31` </location>
<code_context>
- </div>
+ <form action="{% url 'presale:locale.set' %}" method="get" class="locales" aria-label="{% trans "select language" %}">
+ <label for="locale-select-org" class="sr-only">{% trans "Language" %}</label>
+ <select id="locale-select-org" name="locale" onchange="this.form.submit()" class="form-control input-sm">
+ {% for l in languages %}
+ <option value="{{ l.code }}" {% if l.code == request.LANGUAGE_CODE %}selected{% endif %}>{{ l.name_local }}</option>
</code_context>
<issue_to_address>
**suggestion:** Relying solely on `onchange` for form submission may be suboptimal for accessibility.
`onchange="this.form.submit()"` triggers submission as soon as the selection changes, without any explicit submit control. This can be confusing for keyboard and assistive-technology users who expect to confirm with Enter or by moving focus. Please add an accessible submit mechanism (for example, a visually hidden submit button or equivalent) so standard keyboard activation patterns are supported across browsers and AT.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| <option value="{{ l.code }}" {% if l.code == request.LANGUAGE_CODE %}selected{% endif %}>{{ l.name_local }}</option> | ||
| {% endfor %} | ||
| </select> | ||
| <input type="hidden" name="next" value="{{ request.path }}{% if request.META.QUERY_STRING %}?{{ request.META.QUERY_STRING|urlencode }}{% endif %}"> |
Contributor
There was a problem hiding this comment.
suggestion: Consider deduplicating the locale switcher markup and URL-building logic.
The language switcher form and next parameter construction are now repeated in multiple templates (event/base.html twice and organizers/base.html twice). Moving this into a shared include/fragment or a helper for generating next would reduce duplication and keep behavior consistent across templates.
Suggested implementation:
{% include "pretixpresale/partials/locale_switcher.html" %}
To fully implement the deduplication and keep behavior consistent:
- Create a new template fragment at
app/eventyay/presale/templates/pretixpresale/partials/locale_switcher.htmlwith the full, DRY implementation:{% if event.settings.locales|length > 1 %} <form action="{% url 'presale:locale.set' %}" method="get" class="locales" aria-label="{% trans "select language" %}"> <label for="locale-select" class="sr-only">{% trans "Language" %}</label> <select id="locale-select" name="locale" onchange="this.form.submit()" class="form-control input-sm"> {% for l in languages %} <option value="{{ l.code }}" {% if l.code == request.LANGUAGE_CODE %}selected{% endif %}>{{ l.name_local }}</option> {% endfor %} </select> <input type="hidden" name="next" value="{{ request.path }}{% if request.META.QUERY_STRING %}?{{ request.META.QUERY_STRING|urlencode }}{% endif %}"> </form> {% endif %}
- Replace the other three duplicated locale-switcher blocks in
event/base.htmlandorganizers/base.htmlwith{% include "pretixpresale/partials/locale_switcher.html" %}in the same way as above. - If different contexts use different
idvalues for the<select>(e.g., multiple switchers on one page), either:- Parameterize the
idvia{% include ... with select_id="locale-select-header" %}, or - Remove the hardcoded
idif it's not relied on by JavaScript or labels elsewhere.
- Parameterize the
Contributor
There was a problem hiding this comment.
Pull request overview
This PR modernizes the public presale header layout by aligning logos and content with the main container, replacing language links with accessible dropdown selectors, and removing inline styles in favor of CSS classes.
- Wraps event logos in a container div to align with content gutters and removes automatic border-radius
- Replaces inline language links with accessible
<select>dropdowns across event and organizer templates - Removes inline margin styles from login status fragment and adds CSS class for proper alignment
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
app/eventyay/static/pretixpresale/scss/_event.scss |
Removes empty ruleset, resets logo margins to align with container, removes border-radius from logos, and adds new CSS class for login header alignment |
app/eventyay/presale/templates/pretixpresale/organizers/base.html |
Replaces language links with dropdown forms in two locations, adds container wrapper for alignment consistency |
app/eventyay/presale/templates/pretixpresale/event/base.html |
Wraps event logo in container div, replaces language links with dropdown forms in two locations |
app/eventyay/presale/templates/pretixpresale/fragment_login_status.html |
Removes conditional inline margin style and adds CSS class for consistent alignment |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+53
to
65
| <div class="container"> | ||
| <div class="{% if not organizer_logo or not organizer.settings.organizer_logo_image_large %}pull-right flip{% endif %} loginbox"> | ||
| <form action="{% url 'presale:locale.set' %}" method="get" class="locales" aria-label="{% trans "select language" %}"> | ||
| <label for="locale-select-org-inline" class="sr-only">{% trans "Language" %}</label> | ||
| <select id="locale-select-org-inline" name="locale" onchange="this.form.submit()" class="form-control input-sm"> | ||
| {% for l in languages %} | ||
| <option value="{{ l.code }}" {% if l.code == request.LANGUAGE_CODE %}selected{% endif %}>{{ l.name_local }}</option> | ||
| {% endfor %} | ||
| </select> | ||
| <input type="hidden" name="next" value="{{ request.path }}{% if request.META.QUERY_STRING %}?{{ request.META.QUERY_STRING|urlencode }}{% endif %}"> | ||
| </form> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
The language selector in the organizer template has been wrapped in a container div for alignment purposes, but the equivalent language selector in the event template (lines 78-88) lacks this container wrapper. This creates an inconsistency in how the two templates handle the same layout scenario. Consider adding the same container wrapper to the event template's language selector to maintain consistent alignment behavior across both templates.
mariobehling
changed the title
mariobehling
requested changes
Dec 19, 2025
Member
mariobehling
left a comment
mariobehling
left a comment
There was a problem hiding this comment.
Hi @soumyalimje Thanks for your contribution. I updated your title (removed issue number) and fixed the description to help you get started. Please add screenshots and address AI reviews.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixes #1364
Align the public ticket shop header by wrapping the event/organizer logos in the page container so the logo sits flush with the white content area and removing automatic rounding (no border-radius). Replace inline language links with an accessible dropdown that submits to the existing locale setter (preserving the next parameter). Remove inline styles used for header alignment and instead handle alignment via CSS and container placement so the logged-in user name/email lines up with the right edge of the main content area. Includes small SCSS cleanup and layout adjustments.
Summary by Sourcery
Align the public presale header layout so logos and account info line up with the main content area and modernize language selection.
New Features:
Bug Fixes:
Enhancements: