feat(enext): unify event page #1223
Conversation
Reviewer's GuideThis PR refactors and unifies the event page navigation and header across presale, agenda, and base templates by replacing deprecated button styles with a cohesive ‘header-tab’ system, restructuring the header layout, adding consistent inline styling, and synchronizing the user menu in popover.js. Class diagram for updated navigation and profile menu structureclassDiagram
class Navigation {
+header-tab: style
+Tickets: link
+Schedule: link
+Sessions: link
+Speakers: link
+Call for Speakers: link (conditional)
+Videos: link
}
class ProfileMenu {
+My Account: link
+My Talk Profile: link
+My Orders: link
+My Proposals: link
+My Emails: link
+Organizer Area: link (conditional)
+Logout: action
}
Navigation "1" -- "1" ProfileMenu: integrates
ProfileMenu <|-- OrganizerArea: conditional access
ProfileMenu <|-- Logout: action
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}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `app/eventyay/presale/templates/pretixpresale/event/fragment_nav.html:12` </location>
<code_context>
+ <i class="fa fa-info-circle"></i> {% translate "Info" %}
+ </a> {% endcomment %}
+
+ <a target="_blank" class="header-tab underline">
+ <i class="fa fa-ticket"></i> {% translate "Tickets" %}
+ </a>
</code_context>
<issue_to_address>
**issue (bug_risk):** The Tickets link lacks an href attribute, which may confuse users and break navigation.
Please add an href to the Tickets link, or clarify if its absence is intentional.
</issue_to_address>
### Comment 2
<location> `app/eventyay/presale/templates/pretixpresale/event/fragment_nav.html:30-37` </location>
<code_context>
- <i class="fa fa-ticket"></i> {% translate "Tickets" %}
+
+ {% with cfp=request.event.cfp %}
+ <a class="header-tab {% if not cfp.is_open and not access_code.is_valid %}disabled{% endif %}"
+ href="{{ request.event.urls.submit }}{{ submit_qs }}">
+ {% if cfp.is_open or access_code.is_valid %}
+ <i class="fa fa-bullhorn"></i> {% translate "Call for Speakers" %}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The 'disabled' class on anchor tags does not prevent interaction or navigation.
To ensure the link is truly inactive, use JavaScript to block navigation or render it without an href, or as a <span> element.
```suggestion
{% with cfp=request.event.cfp %}
{% if cfp.is_open or access_code.is_valid %}
<a class="header-tab"
href="{{ request.event.urls.submit }}{{ submit_qs }}">
<i class="fa fa-bullhorn"></i> {% translate "Call for Speakers" %}
</a>
{% else %}
<span class="header-tab disabled" tabindex="-1" aria-disabled="true" style="pointer-events: none; color: #aaa;">
<i class="fa fa-bullhorn"></i> {% translate "Call for Speakers" %}
</span>
{% endif %}
{% endwith %}
```
</issue_to_address>
### Comment 3
<location> `app/eventyay/presale/templates/pretixpresale/fragment_login_status.html:10` </location>
<code_context>
{{ 'popover-profile'|json_script:"popover_toggle" }}
{{ base_path|json_script:"base_path" }}
-<nav class="login-hdr" aria-label='{% translate "account" %}'>
+<nav class="login-hdr" aria-label="{% translate 'account' %}" style="{% if request.event and request.event.settings.event_logo_image %}margin-right: 18rem{% else %}margin-right: 0{% endif %}">
+
{% if request.user.is_authenticated %}
</code_context>
<issue_to_address>
**suggestion:** Inline style for margin-right may cause layout issues on smaller screens.
Use responsive CSS classes or media queries to ensure margin-right adapts to various screen sizes for consistent header layout.
Suggested implementation:
```
<nav class="login-hdr{% if request.event and request.event.settings.event_logo_image %} login-hdr--with-logo{% endif %}" aria-label="{% translate 'account' %}">
```
You must add the following CSS to your main stylesheet (e.g., `app/eventyay/presale/static/pretixpresale/css/style.css`):
```css
.login-hdr--with-logo {
margin-right: 18rem;
}
@media (max-width: 768px) {
.login-hdr--with-logo {
margin-right: 0;
}
}
```
This ensures the margin is only applied on larger screens and adapts responsively for smaller screens.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| <i class="fa fa-info-circle"></i> {% translate "Info" %} | ||
| </a> {% endcomment %} | ||
|
|
||
| <a target="_blank" class="header-tab underline"> |
There was a problem hiding this comment.
issue (bug_risk): The Tickets link lacks an href attribute, which may confuse users and break navigation.
Please add an href to the Tickets link, or clarify if its absence is intentional.
| {{ 'popover-profile'|json_script:"popover_toggle" }} | ||
| {{ base_path|json_script:"base_path" }} | ||
| <nav class="login-hdr" aria-label='{% translate "account" %}'> | ||
| <nav class="login-hdr" aria-label="{% translate 'account' %}" style="{% if request.event and request.event.settings.event_logo_image %}margin-right: 18rem{% else %}margin-right: 0{% endif %}"> |
There was a problem hiding this comment.
suggestion: Inline style for margin-right may cause layout issues on smaller screens.
Use responsive CSS classes or media queries to ensure margin-right adapts to various screen sizes for consistent header layout.
Suggested implementation:
<nav class="login-hdr{% if request.event and request.event.settings.event_logo_image %} login-hdr--with-logo{% endif %}" aria-label="{% translate 'account' %}">
You must add the following CSS to your main stylesheet (e.g., app/eventyay/presale/static/pretixpresale/css/style.css):
.login-hdr--with-logo {
margin-right: 18rem;
}
@media (max-width: 768px) {
.login-hdr--with-logo {
margin-right: 0;
}
}This ensures the margin is only applied on larger screens and adapts responsively for smaller screens.
Gagan-Ram
force-pushed
the
1079_unify_event_page
branch
from
974efb9 to
d423e24
Compare
|
Looks good. Please ensure all pages have the same width and logo position. |
There was a problem hiding this comment.
Pull Request Overview
This PR unifies the event page navigation by consolidating header tabs across presale, agenda, and base templates. The changes introduce a consistent navigation structure featuring Tickets, Schedule, Sessions, Speakers, Call for Speakers, and Videos tabs, while also expanding the profile menu to include My Account, My Talk Profile, My Orders, My Proposals, My Emails, Organizer area, and Logout options.
Key Changes:
- Unified navigation header with tabs replacing previous button-based navigation
- Expanded profile dropdown and popover menus with additional user account options
- Updated base template header layout to display event logo with adjusted styling
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| app/eventyay/static/pretixpresale/js/ui/popover.js | Added new profile menu items (My Account, My Talk Profile, My Proposals, My Emails) and updated organizer area URL path |
| app/eventyay/presale/templates/pretixpresale/fragment_login_status.html | Added conditional inline margin styling based on event logo presence |
| app/eventyay/presale/templates/pretixpresale/event/fragment_nav.html | Replaced button-based navigation with tab-based navigation, added inline CSS for underline effects, and included Call for Speakers conditional tab |
| app/eventyay/common/templates/common/base.html | Restructured header layout with event logo positioning, reordered profile dropdown menu items, and updated organizer area URL |
| app/eventyay/agenda/templates/agenda/base.html | Added unified navigation tabs consistent with presale template including Call for Speakers tab |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| </a> | ||
| </div> | ||
| <div class="profile-menu"> | ||
| <a href="${basePath}/${eventSlug}/me" target="_self" class="btn btn-outline-success"> |
There was a problem hiding this comment.
The URL pattern is incomplete - it's missing the organizer slug. According to the URL routing patterns, event URLs should follow the format /{organizer}/{event}/.... The correct URL should be ${basePath}/${organizerName}/${eventSlug}/me/ (note the addition of organizerName and trailing slash). The organizerName variable is already available in scope (lines 77-82).
| <div class="profile-menu"> | ||
| <a href="${basePath}${profilePath}" target="_self" class="btn btn-outline-success"> | ||
| <i class="fa fa-user"></i> ${window.gettext('My Account')} | ||
| <a href="${basePath}/${eventSlug}/me/submissions/" target="_self" class="btn btn-outline-success"> |
There was a problem hiding this comment.
The URL pattern is incomplete - it's missing the organizer slug. The correct URL should be ${basePath}/${organizerName}/${eventSlug}/me/submissions/ to match the expected URL routing pattern /{organizer}/{event}/me/submissions/.
| <a href="${basePath}/${eventSlug}/me/submissions/" target="_self" class="btn btn-outline-success"> | |
| <a href="${basePath}/${organizerName}/${eventSlug}/me/submissions/" target="_self" class="btn btn-outline-success"> |
| </a> | ||
| </div> | ||
| <div class="profile-menu"> | ||
| <a href="${basePath}/${eventSlug}/me/mails/" target="_self" class="btn btn-outline-success"> |
There was a problem hiding this comment.
The URL pattern is incomplete - it's missing the organizer slug. The correct URL should be ${basePath}/${organizerName}/${eventSlug}/me/mails/ to match the expected URL routing pattern /{organizer}/{event}/me/mails/.
| <a href="${basePath}/${eventSlug}/me/mails/" target="_self" class="btn btn-outline-success"> | |
| <a href="${basePath}/${organizerName}/${eventSlug}/me/mails/" target="_self" class="btn btn-outline-success"> |
| <i class="fa fa-info-circle"></i> {% translate "Info" %} | ||
| </a> {% endcomment %} | ||
|
|
||
| <a target="_blank" class="header-tab underline"> |
There was a problem hiding this comment.
Using target=\"_blank\" for internal navigation is inappropriate and creates a poor user experience. This link appears to be for the Tickets tab within the same application. Remove the target=\"_blank\" attribute to allow normal in-page navigation.
| <a target="_blank" class="header-tab underline"> | |
| <a class="header-tab underline"> |
| {% comment %} <a href="" class="header-tab {% if request.path_info == '/' or request.path_info == '/tickets' %}underline{% endif %}"> | ||
| <i class="fa fa-info-circle"></i> {% translate "Info" %} | ||
| </a> {% endcomment %} | ||
|
|
There was a problem hiding this comment.
Remove commented-out code rather than leaving it in the codebase. If this Info tab feature might be needed in the future, document it in an issue or task tracker instead.
| {% comment %} <a href="" class="header-tab {% if request.path_info == '/' or request.path_info == '/tickets' %}underline{% endif %}"> | |
| <i class="fa fa-info-circle"></i> {% translate "Info" %} | |
| </a> {% endcomment %} |
| {% comment %} <a href="{}" class="header-tab"> | ||
| <i class="fa fa-info-circle"></i> {% translate "Info" %} | ||
| </a> {% endcomment %} |
There was a problem hiding this comment.
Remove commented-out code rather than leaving it in the codebase. If this Info tab feature might be needed in the future, document it in an issue or task tracker instead.
| {% comment %} <a href="{}" class="header-tab"> | |
| <i class="fa fa-info-circle"></i> {% translate "Info" %} | |
| </a> {% endcomment %} |
|
@Gagan-Ram Please implement it in a way that in the default start view we still have the menu overlapping the background header as shown in the below image. 
|
Gagan-Ram
force-pushed
the
1079_unify_event_page
branch
from
aabfd28 to
414bcd3
Compare
new page 
|
Gagan-Ram
force-pushed
the
1079_unify_event_page
branch
from
414bcd3 to
6962a86
Compare
Gagan-Ram
force-pushed
the
1079_unify_event_page
branch
from
6962a86 to
1dd861b
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {{ 'popover-profile'|json_script:"popover_toggle" }} | ||
| {{ base_path|json_script:"base_path" }} | ||
| <nav class="login-hdr" aria-label='{% translate "account" %}'> | ||
| <nav class="login-hdr" aria-label="{% translate 'account' %}" style="{% if request.event and request.event.settings.event_logo_image %}margin-right: 18rem{% else %}margin-right: 0{% endif %}"> |
There was a problem hiding this comment.
Inconsistent quote usage in HTML attributes. The opening quote is a double quote (") while the closing quote is a single quote ('). Both should be double quotes for consistency: aria-label="{% translate 'account' %}".
| </div> | ||
| {% endif %} | ||
| <div class="container{% block container_width %}{% endblock container_width %}" id="main-container"> | ||
| <div class="container{% block container_width %}{% endblock container_width %}" id="main-container" style="margin-top: 3.8rem"> |
There was a problem hiding this comment.
[nitpick] Inline styles should be moved to CSS files for better maintainability. The PR description mentions this as future work, but adding more inline styles increases technical debt. Consider defining this style in _layout.css.
| <div class="container{% block container_width %}{% endblock container_width %}" id="main-container" style="margin-top: 3.8rem"> | |
| <div class="container{% block container_width %}{% endblock container_width %}" id="main-container"> |
| <div class="event-logo"> | ||
| <a href="{% block nav_link %}{% endblock nav_link %}"> | ||
| {% if request.event and request.event.logo %} | ||
| <img loading="lazy" src="{{ request.event.logo.url }}" id="event-logo" alt="{% translate "The event’s logo" %}" style="border-radius: 10px;"> |
There was a problem hiding this comment.
[nitpick] Inline styles should be moved to CSS files for better maintainability. The PR description mentions this as future work, but adding more inline styles increases technical debt. Consider defining this style in _layout.css.
| <img loading="lazy" src="{{ request.event.logo.url }}" id="event-logo" alt="{% translate "The event’s logo" %}" style="border-radius: 10px;"> | |
| <img loading="lazy" src="{{ request.event.logo.url }}" id="event-logo" alt="{% translate "The event’s logo" %}" class="event-logo-img"> |
| } | ||
|
|
||
| .header-wrapper { | ||
| overflow: scroll; |
There was a problem hiding this comment.
The overflow: scroll property will always show scrollbars even when content doesn't overflow. Consider using overflow: auto instead, which only shows scrollbars when necessary.
| overflow: scroll; | |
| overflow: auto; |
| <a class="header-tab {% if not cfp.is_open and not access_code.is_valid %}disabled{% endif %}" | ||
| href="{{ request.event.urls.submit }}{{ submit_qs }}"> | ||
| {% if cfp.is_open or access_code.is_valid %} | ||
| <i class="fa fa-bullhorn"></i> {% translate "Call for Speakers" %} | ||
| {% endif %} | ||
| </a> |
There was a problem hiding this comment.
When the CFP is closed or the access code is invalid, the link shows empty content because the icon and text are inside the conditional block. This creates an empty, disabled link in the navigation. Consider moving the entire <a> tag inside the conditional, or display the text outside the conditional with a disabled state message.
| <a class="header-tab {% if not cfp.is_open and not access_code.is_valid %}disabled{% endif %}" | |
| href="{{ request.event.urls.submit }}{{ submit_qs }}"> | |
| {% if cfp.is_open or access_code.is_valid %} | |
| <i class="fa fa-bullhorn"></i> {% translate "Call for Speakers" %} | |
| {% endif %} | |
| </a> | |
| {% if cfp.is_open or access_code.is_valid %} | |
| <a class="header-tab" | |
| href="{{ request.event.urls.submit }}{{ submit_qs }}"> | |
| <i class="fa fa-bullhorn"></i> {% translate "Call for Speakers" %} | |
| </a> | |
| {% else %} | |
| <span class="header-tab disabled" title="{% translate 'Call for Speakers is closed' %}"> | |
| <i class="fa fa-bullhorn"></i> {% translate "Call for Speakers" %} | |
| </span> | |
| {% endif %} |
| <a class="header-tab {% if not cfp.is_open and not access_code.is_valid %}disabled{% endif %}" | ||
| href="{{ request.event.urls.submit }}{{ submit_qs }}"> | ||
| {% if cfp.is_open or access_code.is_valid %} | ||
| <i class="fa fa-bullhorn"></i> {% translate "Call for Speakers" %} | ||
| {% endif %} | ||
| </a> |
There was a problem hiding this comment.
When the CFP is closed or the access code is invalid, the link shows empty content because the icon and text are inside the conditional block. This creates an empty, disabled link in the navigation. Consider moving the entire <a> tag inside the conditional, or display the text outside the conditional with a disabled state message.
| <a class="header-tab {% if not cfp.is_open and not access_code.is_valid %}disabled{% endif %}" | |
| href="{{ request.event.urls.submit }}{{ submit_qs }}"> | |
| {% if cfp.is_open or access_code.is_valid %} | |
| <i class="fa fa-bullhorn"></i> {% translate "Call for Speakers" %} | |
| {% endif %} | |
| </a> | |
| {% if cfp.is_open or access_code.is_valid %} | |
| <a class="header-tab {% if "/cfp/" in request.path_info %}active{% endif %}" | |
| href="{{ request.event.urls.submit }}{{ submit_qs }}"> | |
| <i class="fa fa-bullhorn"></i> {% translate "Call for Speakers" %} | |
| </a> | |
| {% else %} | |
| <a class="header-tab disabled" tabindex="-1" aria-disabled="true" href="#"> | |
| <i class="fa fa-bullhorn"></i> {% translate "Call for Speakers (Closed)" %} | |
| </a> | |
| {% endif %} |
|
|
||
| .public-event-header { | ||
| display: flex; | ||
| margin-bottom: 14px |
There was a problem hiding this comment.
Missing semicolon at the end of the CSS declaration. This should be margin-bottom: 14px; to follow CSS syntax standards.
| margin-bottom: 14px | |
| margin-bottom: 14px; |
| <style> | ||
| .navigation a { | ||
| cursor: pointer; | ||
| position: relative; | ||
| text-decoration: none; | ||
| padding-bottom: 4px; | ||
| } | ||
|
|
||
| .navigation a.underline::after { | ||
| content: ''; | ||
| position: absolute; | ||
| bottom: 0; | ||
| left: -10px; | ||
| width: calc(100% + 20px); | ||
| height: 2px; | ||
| background-color: currentColor; | ||
| } | ||
|
|
||
| .navigation a:not(.underline):hover { | ||
| border-bottom: 2px dotted currentColor; | ||
| } | ||
|
|
||
| </style> |
There was a problem hiding this comment.
[nitpick] Inline styles embedded in <style> tags within templates should be moved to external CSS files for better maintainability, reusability, and caching. The PR description mentions this as future work, but consider creating a dedicated CSS file for navigation styles now to avoid increasing technical debt.
| {% endif %} | ||
| </div> | ||
| <header {% if event_logo %} class="custom-header" {% endif %}> | ||
| <header {% if event_logo %} class="custom-header" {% endif %} style="margin-top: -3.8rem;"> |
There was a problem hiding this comment.
[nitpick] Inline styles should be moved to CSS files for better maintainability. The PR description mentions this as future work, but adding more inline styles increases technical debt. Consider defining this style in _layout.css.
| <header {% if event_logo %} class="custom-header" {% endif %} style="margin-top: -3.8rem;"> | |
| <header {% if event_logo %} class="custom-header header-margin-top" {% endif %}> |
| {{ 'popover-profile'|json_script:"popover_toggle" }} | ||
| {{ base_path|json_script:"base_path" }} | ||
| <nav class="login-hdr" aria-label='{% translate "account" %}'> | ||
| <nav class="login-hdr" aria-label="{% translate 'account' %}" style="{% if request.event and request.event.settings.event_logo_image %}margin-right: 18rem{% else %}margin-right: 0{% endif %}"> |
There was a problem hiding this comment.
[nitpick] Inline styles should be moved to CSS files for better maintainability. The PR description mentions moving inline CSS to .css files as a future task, but this adds more inline styles. Consider defining a CSS class in _layout.css instead.
| <nav class="login-hdr" aria-label="{% translate 'account' %}" style="{% if request.event and request.event.settings.event_logo_image %}margin-right: 18rem{% else %}margin-right: 0{% endif %}"> | |
| <nav class="login-hdr{% if request.event and request.event.settings.event_logo_image %} login-hdr--with-logo{% endif %}" aria-label="{% translate 'account' %}"> |
| {% comment %} <a href="{}" class="header-tab"> | ||
| <i class="fa fa-info-circle"></i> {% translate "Info" %} | ||
| </a> {% endcomment %} |
There was a problem hiding this comment.
Your Call for speakers links directly to the submission form instead of the Call for Speakers/Proposals, e.g. https://next.eventyay.com/a-team/summit/cfp
This page also needs the header menu!
mariobehling
left a comment
mariobehling
left a comment
There was a problem hiding this comment.
Please show a button on the right "Join online event" instead of the "Videos" button.
Compare:
|
Please provide screenshots of updates. |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <nav style="margin-top: 10px; margin-bottom: 3rem; overflow: scroll;"> | ||
| <div class="navigation" style="font-size: 1.65rem; display: flex; gap: 2rem;"> |
There was a problem hiding this comment.
Multiple inline styles should be moved to external CSS files as noted in the PR description. These styles (margin-top, margin-bottom, overflow, font-size, display, gap) would be more maintainable in a CSS class.
| <a class="header-tab underline"> | ||
| <i class="fa fa-ticket"></i> {% translate "Tickets" %} | ||
| </a> |
There was a problem hiding this comment.
The 'Tickets' link is missing an href attribute, making it non-functional. This appears to be the active/current page indicator, but it should still be a valid link or use a different element (like ) to avoid misleading screen readers and keyboard users.
| <style> | ||
| .navigation a { | ||
| cursor: pointer; | ||
| position: relative; | ||
| text-decoration: none; | ||
| padding-bottom: 4px; | ||
| } | ||
|
|
||
| .navigation a.underline::after { | ||
| content: ''; | ||
| position: absolute; | ||
| bottom: 0; | ||
| left: -10px; | ||
| width: calc(100% + 20px); | ||
| height: 2px; | ||
| background-color: currentColor; | ||
| } | ||
|
|
||
| .navigation a:not(.underline):hover { | ||
| border-bottom: 2px dotted currentColor; | ||
| } | ||
|
|
||
| @media (min-width: 375px) { | ||
| .video-link { | ||
| margin-left: 0; | ||
| } | ||
| } | ||
|
|
||
| @media (min-width: 1000px) { | ||
| .video-link { | ||
| margin-left: 20rem; | ||
| } | ||
| } | ||
|
|
||
| @media (min-width: 1200px) { | ||
| .video-link { | ||
| margin-left: 40rem; | ||
| } | ||
| } | ||
| </style> |
There was a problem hiding this comment.
This entire <style> block should be moved to the external CSS file (_layout.css) as mentioned in the PR description. Inline <style> blocks in templates make styles harder to maintain and prevent proper CSS optimization.
| <style> | |
| .navigation a { | |
| cursor: pointer; | |
| position: relative; | |
| text-decoration: none; | |
| padding-bottom: 4px; | |
| } | |
| .navigation a.underline::after { | |
| content: ''; | |
| position: absolute; | |
| bottom: 0; | |
| left: -10px; | |
| width: calc(100% + 20px); | |
| height: 2px; | |
| background-color: currentColor; | |
| } | |
| .navigation a:not(.underline):hover { | |
| border-bottom: 2px dotted currentColor; | |
| } | |
| @media (min-width: 375px) { | |
| .video-link { | |
| margin-left: 0; | |
| } | |
| } | |
| @media (min-width: 1000px) { | |
| .video-link { | |
| margin-left: 20rem; | |
| } | |
| } | |
| @media (min-width: 1200px) { | |
| .video-link { | |
| margin-left: 40rem; | |
| } | |
| } | |
| </style> |
| <div class="event-logo"> | ||
| <a href="{% block nav_link %}{% endblock nav_link %}"> | ||
| {% if request.event and request.event.logo %} | ||
| <img loading="lazy" src="{{ request.event.logo.url }}" id="event-logo" alt="{% translate "The event’s logo" %}" style="border-radius: 10px;"> |
There was a problem hiding this comment.
Inline style should be moved to CSS file. The border-radius styling would be more maintainable in the external CSS, potentially as part of the #event-logo selector that already exists.
| <img loading="lazy" src="{{ request.event.logo.url }}" id="event-logo" alt="{% translate "The event’s logo" %}" style="border-radius: 10px;"> | |
| <img loading="lazy" src="{{ request.event.logo.url }}" id="event-logo" alt="{% translate "The event’s logo" %}"> |
| .video-link { | ||
| margin-left: 24rem; | ||
| } |
There was a problem hiding this comment.
The .video-link class has multiple conflicting margin-left values across different media queries (24rem at line 221, 20rem at line 229, 10rem at line 238, 0rem at lines 251, 342). The base value of 24rem is only used above 1200px width, but it's defined outside any media query. Consider restructuring to have the default (mobile-first) value first, then progressively enhance for larger screens.
| .header-wrapper { | ||
| overflow: auto; | ||
| background: var(--color-bg); | ||
| padding-bottom: 6rem; |
There was a problem hiding this comment.
The padding-bottom of 6rem inside a max-width: 576px media query seems excessively large and could cause significant whitespace issues on mobile devices. Consider reducing this value or verify if this is the intended behavior.
| padding-bottom: 6rem; | |
| padding-bottom: 2rem; |
| {% endcompress %} | ||
| {% endblock cfp_header %} | ||
|
|
||
|
|
There was a problem hiding this comment.
[nitpick] Unnecessary blank line addition. This change adds no functional value and should be removed to keep the codebase clean.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {% endwith %} | ||
|
|
||
| <div class="video-link"> | ||
| <a join-event-link class="header-tab join-event {% if request.path_info == '/onlinevideo' %}underline{% endif %}" |
There was a problem hiding this comment.
Invalid HTML attribute 'join-event-link' without a value. This should either be removed, prefixed with 'data-' (e.g., 'data-join-event-link'), or given a value if it's meant to be an ID attribute (e.g., 'id="join-event-link"').
| <a join-event-link class="header-tab join-event {% if request.path_info == '/onlinevideo' %}underline{% endif %}" | |
| <a data-join-event-link class="header-tab join-event {% if request.path_info == '/onlinevideo' %}underline{% endif %}" |
| {% endif %} | ||
| </div> | ||
| <header {% if event_logo %} class="custom-header" {% endif %}> | ||
| <header {% if event_logo %} class="custom-header" {% endif %} style="margin-top: -3.8rem;"> |
There was a problem hiding this comment.
Inline style should be moved to CSS as mentioned in the PR description. The negative margin should be defined in the CSS file, either as part of the 'custom-header' class or through a modifier class.
| <header {% if event_logo %} class="custom-header" {% endif %} style="margin-top: -3.8rem;"> | |
| <header{% if event_logo %} class="custom-header"{% endif %}> |
| <a id="join-event-link" href='{% url "agenda:event.onlinevideo.join" organizer=request.event.organizer.slug event=request.event.slug %}' class="header-tab video-link"> | ||
| <i class="fa fa-video-camera"></i> {% translate "Join online video" %} | ||
| </a> |
There was a problem hiding this comment.
The 'video-link' class and 'join-event-link' id appear to have overlapping purposes. In the presale template, 'video-link' is used as a wrapper div class, while here it's applied directly to the anchor. This inconsistency could cause styling issues and should be unified across templates.
| <a id="join-event-link" href='{% url "agenda:event.onlinevideo.join" organizer=request.event.organizer.slug event=request.event.slug %}' class="header-tab video-link"> | |
| <i class="fa fa-video-camera"></i> {% translate "Join online video" %} | |
| </a> | |
| <div class="video-link"> | |
| <a id="join-event-link" href='{% url "agenda:event.onlinevideo.join" organizer=request.event.organizer.slug event=request.event.slug %}' class="header-tab"> | |
| <i class="fa fa-video-camera"></i> {% translate "Join online video" %} | |
| </a> | |
| </div> |
Gagan-Ram
force-pushed
the
1079_unify_event_page
branch
from
8a16740 to
79f3261
Compare
4 participants
fixes: #1079
Screencast.from.2025-11-11.01-26-17.webm
@mariobehling Please check the latest UI as discussed earlier. once approved I can work on moving inline css to .css files
Summary by Sourcery
Unify event page header and navigation across presale, agenda, and base templates and update profile menu in popover JS
Enhancements: