fix for Match event name header font and size on Tickets and Common Dashboard to Talk Dashboard #1459
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
Signed-off-by: Vedansh Saini <77830698+vedansh-5@users.noreply.github.com>
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAligns the event header hierarchy, font styling, and spacing across the Tickets Dashboard, Common Dashboard, and Talk Dashboard to be consistent in size and appearance, especially for the event name and date/series label. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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:
- Consider avoiding the new
<br>for vertical spacing inpretixcontrol/event/index.htmland instead rely on existing layout classes or CSS margins to keep spacing concerns out of the markup. - In
orga/event/dashboard.htmlthe inlinestyle="font-size: 1.25rem;"on the date span diverges from the other dashboards; it would be more maintainable to reuse the samesmall/text-mutedclasses or a shared CSS class instead of hardcoded inline styles. - The change from
<nav>to<div>for#event-navin the control template reduces semantic clarity and also makes this template inconsistent with the common dashboard, which still uses<nav>; consider keeping a semantic<nav>for the header navigation across dashboards.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider avoiding the new `<br>` for vertical spacing in `pretixcontrol/event/index.html` and instead rely on existing layout classes or CSS margins to keep spacing concerns out of the markup.
- In `orga/event/dashboard.html` the inline `style="font-size: 1.25rem;"` on the date span diverges from the other dashboards; it would be more maintainable to reuse the same `small`/`text-muted` classes or a shared CSS class instead of hardcoded inline styles.
- The change from `<nav>` to `<div>` for `#event-nav` in the control template reduces semantic clarity and also makes this template inconsistent with the common dashboard, which still uses `<nav>`; consider keeping a semantic `<nav>` for the header navigation across dashboards.
## Individual Comments
### Comment 1
<location> `app/eventyay/control/templates/pretixcontrol/event/index.html:10-19` </location>
<code_context>
+ <div id="event-nav" class="header-nav">
</code_context>
<issue_to_address>
**issue (bug_risk):** Replacing the semantic <nav> landmark with a generic <div> may hurt accessibility and navigation structure.
The previous `<nav id="event-nav" class="header-nav">` exposed a navigation landmark that screen readers and some browser tools use for quick navigation. Switching to a `<div>` removes that semantic. Unless there’s a specific conflict (e.g., invalid nested `<nav>`), please keep `<nav>` or add an equivalent landmark such as `role="navigation"` so this remains accessible.
</issue_to_address>
### Comment 2
<location> `app/eventyay/control/templates/pretixcontrol/event/index.html:114` </location>
<code_context>
{% include "pretixcontrol/event/fragment_subevent_choice_simple.html" %}
</form>
{% endif %}
+ <br>
<div class="dashboard custom">
{% if not request.event.has_subevents or subevent %}
</code_context>
<issue_to_address>
**suggestion:** Using a bare <br> for vertical spacing is brittle; layout is better handled via CSS.
Here it’s only adding visual spacing, which couples layout to markup and may break with responsive changes. Prefer handling this via CSS (e.g., margin on the surrounding container or a utility/spacing class) to keep structure and presentation separate.
Suggested implementation:
```
{% endif %}
<div class="dashboard custom mt-3">
```
1. Ensure that the `mt-3` class (or equivalent) exists and is appropriate in your CSS framework (it likely comes from Bootstrap or a project-specific utility set).
2. If `mt-3` is not available or the spacing needs tuning, either:
- Adjust to a different existing margin utility (e.g., `mt-2`, `mt-4`), or
- Define a custom class (e.g., `.dashboard-spacing { margin-top: 1rem; }`) and use `class="dashboard custom dashboard-spacing"` instead.
</issue_to_address>
### Comment 3
<location> `app/eventyay/orga/templates/orga/event/dashboard.html:32` </location>
<code_context>
<span>
{{ request.event.name }}
- <small class="text-muted small">{{ request.event.get_date_range_display }}</small>
+ <span class="text-muted" style="font-size: 1.25rem;">{{ request.event.get_date_range_display }}</span>
</span>
</h2>
</code_context>
<issue_to_address>
**suggestion:** Inline styling for font size makes the date display harder to maintain and inconsistent with the other templates.
This template diverges by using a `<span>` with inline `style="font-size: 1.25rem;"` instead of the existing `<small class="text-muted small">` pattern. Please either align with the `<small>` + Bootstrap classes used elsewhere or move the font size into a shared CSS class so future style changes stay consistent across dashboards.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@mariobehling PTAL thanks! |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR standardizes the event header display across three different dashboards (Tickets, Common, and Talk/Organizer) by matching font sizes and HTML structure. The goal is to achieve visual consistency in how event names and dates are displayed.
Key Changes:
- Changed heading levels from
<h1>to<h2>in Common and Tickets dashboards - Updated font styling for event names and dates
- Modified HTML structure with additional wrapper elements
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| app/eventyay/orga/templates/orga/event/dashboard.html | Added inline styles to control font sizes for event name (1.85rem) and date (1.25rem) |
| app/eventyay/eventyay_common/templates/eventyay_common/event/index.html | Changed heading from <h1> to <h2> and added CSS classes for text styling |
| app/eventyay/control/templates/pretixcontrol/event/index.html | Changed <nav> to <div>, heading from <h1> to <h2>, added <span> wrapper, and included CSS classes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <span style="font-size: 1.85rem;"> | ||
| {{ request.event.name }} | ||
| <small class="text-muted small">{{ request.event.get_date_range_display }}</small> | ||
| <span class="text-muted" style="font-size: 1.25rem;">{{ request.event.get_date_range_display }}</span> |
There was a problem hiding this comment.
Using inline styles for font sizing is not a best practice and creates maintainability issues. Consider using CSS classes instead to maintain consistency with the other dashboard templates (eventyay_common/event/index.html and pretixcontrol/event/index.html) which use the text-muted and small classes. This would also make future styling changes easier to apply across all dashboards.
Suggested change
| <span class="text-muted" style="font-size: 1.25rem;">{{ request.event.get_date_range_display }}</span> | |
| <span class="text-muted small">{{ request.event.get_date_range_display }}</span> |
Comment on lines
+10
to
+28
| <div id="event-nav" class="header-nav"> | ||
| <div class="navigation"> | ||
| <div class="navigation-title"> | ||
| <h1> | ||
| {{ request.event.name }} | ||
| <small> | ||
| {% if request.event.has_subevents %} | ||
| <h2> | ||
| <span> | ||
| {{ request.event.name }} | ||
| <small class="text-muted small"> | ||
| {% if request.event.has_subevents %} | ||
| {% trans "Event series" %} | ||
| {% else %} | ||
| {{ request.event.get_date_range_display }} | ||
| {% endif %} | ||
| </small> | ||
| </h1> | ||
| {% endif %} | ||
| </small> | ||
| </span> | ||
| </h2> | ||
| </div> | ||
| {% include "pretixcontrol/event/component_link.html" %} | ||
| </div> | ||
| </nav> | ||
| </div> |
There was a problem hiding this comment.
Changing the semantic <nav> element to a <div> reduces the accessibility and semantic meaning of the HTML. The <nav> element is important for screen readers and assistive technologies to identify navigation regions. If the styling is the concern, CSS should be adjusted instead of changing the semantic HTML element. Consider keeping <nav> and adjusting styles if needed.
Comment on lines
+13
to
+24
| <h2> | ||
| <span> | ||
| {{ request.event.name }} | ||
| <small class="text-muted small"> | ||
| {% if request.event.has_subevents %} | ||
| {% trans "Event series" %} | ||
| {% else %} | ||
| {{ request.event.get_date_range_display }} | ||
| {% endif %} | ||
| </small> | ||
| </h1> | ||
| {% endif %} | ||
| </small> | ||
| </span> | ||
| </h2> |
There was a problem hiding this comment.
The three dashboard templates now use inconsistent HTML structures to achieve the same visual goal:
orga/event/dashboard.html: Uses inline styles with specific rem valueseventyay_common/event/index.html: Uses CSS classes (text-muted small)pretixcontrol/event/index.html: Uses CSS classes with an additional<span>wrapper
For better maintainability, all three templates should use the same structure and styling approach. Consider standardizing on CSS classes rather than inline styles.
| {{ request.event.name }} | ||
| <small class="text-muted small"> | ||
| {% if request.event.has_subevents %} | ||
| {% trans "Event series" %} |
There was a problem hiding this comment.
The indentation is inconsistent here. Line 18 uses only 12 spaces for indentation while the closing {% else %} on line 19 is properly indented. Line 18 should have consistent indentation with line 20.
Suggested change
| {% trans "Event series" %} | |
| {% trans "Event series" %} |
mariobehling
requested changes
Dec 9, 2025
Member
mariobehling
left a comment
mariobehling
left a comment
There was a problem hiding this comment.
@vedansh-5 Thank you! Please check 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.
Closes #1368
Changes
Summary by Sourcery
Align event header hierarchy and styling across tickets, common, and organizer dashboards for consistent presentation of event name and date.
Bug Fixes:
Enhancements: