fix(ui): match top menu bar between eventyay talk and eventyay tickets components #1259
Conversation
Reviewer's GuideOverhauls CSS/SCSS and template changes to unify typography, icon sizing, spacing, layout, and interactions of the top menu bars across Eventyay Talk and Tickets, ensuring a consistent UI. Class diagram for updated top menu bar CSS classesclassDiagram
class Navbar {
+font-size: 14px
+font-weight: normal
+padding: 0.5rem
+background-color: var(--color-primary)
+border-color: var(--color-border-navbar)
}
class NavbarBrand {
+font-size: 1.25rem / 2rem
+font-weight: normal
+display: flex
+align-items: center
+img.height: 30px
+img.margin-right: 0.5rem
}
class NavLink {
+font-size: 14px
+font-weight: normal
+padding: 0.5rem
+active: font-size: 14px, font-weight: normal
}
class LogoutButton {
+icon-only
+font-size: 14px
+font-weight: normal
+margin: 0 4px
}
Navbar "1" *-- "many" NavbarBrand
Navbar "1" *-- "many" NavLink
Navbar "1" *-- "1" LogoutButton
Class diagram for updated sidebar toggle buttonclassDiagram
class NavbarToggleSidebar {
+display: flex
+align-items: center
+font-size: 14px
+margin-top: 12px
+padding: 0.5rem
+background-color: transparent
+border: none
+color: #fff
}
File-Level Changes
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 - here's some feedback:
- Since the logout button no longer has text, please add an aria-label or visually hidden text for screen readers to maintain accessibility compliance.
- There are many repeated
font-size: 14pxandfont-weight: normaldeclarations across your CSS/SCSS—consider extracting these into a shared SCSS variable or mixin to reduce duplication. - The selector
.navbar-nav .nav-item.d-none.d-md-blockis defined twice with conflictingmargin-bottomvalues; consolidate into a single rule to avoid confusion and ensure consistent spacing.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since the logout button no longer has text, please add an aria-label or visually hidden text for screen readers to maintain accessibility compliance.
- There are many repeated `font-size: 14px` and `font-weight: normal` declarations across your CSS/SCSS—consider extracting these into a shared SCSS variable or mixin to reduce duplication.
- The selector `.navbar-nav .nav-item.d-none.d-md-block` is defined twice with conflicting `margin-bottom` values; consolidate into a single rule to avoid confusion and ensure consistent spacing.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Saksham-Sirohi
left a comment
Saksham-Sirohi
left a comment
There was a problem hiding this comment.
close, but the text next to the logo has different weights, also padding for other icons and items are slightly different on both components, make sure both of them are the same
|
@Saksham-Sirohi can you take a look at these changes 
|
Hi, everything looks fine, just match their font weights as well |
There was a problem hiding this comment.
Pull Request Overview
This PR unifies the visual styling of the top navigation bars between Eventyay Talk and Eventyay Tickets to provide a consistent user experience. The changes standardize typography (font-size: 14px, font-weight: normal), icon sizing (height: 30px for brand logo), and spacing/alignment across both components.
- Standardized all navbar links, buttons, and text to use 14px font size with normal weight
- Updated brand logo size to 30px height and adjusted navbar padding to 1.2rem
- Removed "Log out" button text label, keeping only the icon for consistency
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| app/eventyay/static/pretixcontrol/scss/main.scss | Added comprehensive navbar styling rules for the Tickets component including font standardization, color schemes (#f8f9fa/#fff), and hover states for navbar elements, sidebar toggle, and buttons |
| app/eventyay/static/pretixcontrol/scss/_sidebar.scss | Updated sidebar toggle to use flexbox layout, standardized font-size to 14px, and adjusted margin-top from 8px to 12px for better vertical alignment |
| app/eventyay/static/pretixcontrol/scss/_sb-admin-2.scss | Added font-size: 14px and font-weight: normal to navbar-top-links styling |
| app/eventyay/static/orga/css/_layout.css | Standardized navbar styling for Talk component including brand font-size (20px), padding (1.2rem), global nav menu positioning, and font standardization (14px/normal) for all navbar elements |
| app/eventyay/orga/templates/orga/base.html | Removed "Log out" text span from logout button, keeping only the icon for a cleaner icon-only presentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Saksham-Sirohi
left a comment
Saksham-Sirohi
left a comment
There was a problem hiding this comment.
Font weight for orga(talk component) does not match that of the commons area. Please ensure it follows the font-weight same as of the commons area
|
Hi @Saksham-Sirohi, I’ve used font-weight |
I don't see any changes in your files to the global-nav-menu component; changes on the talk component's navbar are related to there only. Hopefully, this will also help. I noticed the control area lacks a logout button. Please add that as well and ensure consistency there as well |
|
Just to confirm, please remove the logout button on the right-handside of the menu header. |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """ | ||
| Call eventyay-ticket API to check if user owns ticket for this event. | ||
| # NOTE: It doesn't work with the Docker setup for development, because we use fake domain then, |
There was a problem hiding this comment.
The comment has a syntax error. The line "# NOTE: It doesn't work with the Docker setup for development, because we use fake domain then," should be part of the docstring, not a standalone comment starting with "#". This comment line should either be part of the docstring above or properly formatted as a multi-line comment.
| # NOTE: It doesn't work with the Docker setup for development, because we use fake domain then, | |
| NOTE: It doesn't work with the Docker setup for development, because we use a fake domain then, |
prayag78
force-pushed
the
fix/topbar-consistency-eventyay-talk-tickets
branch
from
4a8fd29 to
e64c605
Compare
Saksham-Sirohi
left a comment
Saksham-Sirohi
left a comment
There was a problem hiding this comment.
Hi, upon reviewing the latest changes, I noticed part of the issue demanding similar font-weight on talk and tickets is not satisfied. Also, some font weights may not be supported on the fonts in one of the places we are using. Please ensure this consistency
mariobehling
left a comment
mariobehling
left a comment
There was a problem hiding this comment.
Ok, merging this and we can follow up if any additional changes are needed in follow-up PRs. Thank you!
3 participants
Summary
This pull request fixes the visual inconsistencies between the top menu bars in Eventyay Talk and Eventyay Tickets. The goal is to make both headers look and feel the same when switching between the two. The update focuses on typography, icon sizes, spacing, and alignment.
fixes #1221
Changes
Screenshots
Checklist
Summary by Sourcery
Unify the top menu bar styling between Eventyay Talk and Tickets for consistent typography, icon sizing, spacing, and alignment across desktop and mobile.
Enhancements: