match tickets and common dashboard dropdown to talk dropdown#1376
match tickets and common dashboard dropdown to talk dropdown#1376Aqil-Ahmad wants to merge 10 commits intofossasia:devfrom
Conversation
Reviewer's GuideAligns the Tickets and Common dashboard dropdown styling and structure with the Talk dropdown by introducing shared popover styling, normalizing spacing/typography/icon sizes, and standardizing separators and submenu behavior in both pretixcontrol and pretixpresale stylesheets. Flow diagram for shared dropdown popover styling across dashboardsflowchart TD
User["User clicks profile avatar"]
TalkNavbar["Talk navbar dropdown trigger"]
TicketsNavbar["Tickets navbar dropdown trigger"]
CommonNavbar["Common dashboard navbar dropdown trigger"]
SharedPopover["Shared popover base styles (.popover)"]
SharedContent["Shared popover content styles (.popover-content)"]
SharedProfileMenu["Shared profile-menu styles"]
SharedSubmenu["Shared submenu and submenu-item styles"]
User --> TalkNavbar
User --> TicketsNavbar
User --> CommonNavbar
TalkNavbar --> SharedPopover
TicketsNavbar --> SharedPopover
CommonNavbar --> SharedPopover
SharedPopover --> SharedContent
SharedContent --> SharedProfileMenu
SharedContent --> SharedSubmenu
SharedProfileMenu --> HoverState["Consistent hover background and text color"]
SharedProfileMenu --> Separators["Standardized separators and borders"]
SharedProfileMenu --> Typography["Normalized font size, padding, line height"]
SharedSubmenu --> SubmenuAppearance["Unified submenu spacing, icons, and hover"]
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 - here's some feedback:
- The new
.popoverstyling is duplicated acrosspretixcontrolandpretixpresaleSCSS; consider extracting a shared mixin or partial to keep the dropdown look consistent and easier to maintain. - Using hard-coded
:nth-child(6)/:nth-child(4)selectors for adding borders to specific.profile-menuentries is brittle; tying these styles to semantic classes (e.g.,.primary-actions,.logout) would be more robust against DOM changes. - There are many
!importantoverrides added for padding, borders, and backgrounds in the dropdown; it may be worth revisiting selector specificity or restructuring the CSS so these can be reduced to avoid future override conflicts.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `.popover` styling is duplicated across `pretixcontrol` and `pretixpresale` SCSS; consider extracting a shared mixin or partial to keep the dropdown look consistent and easier to maintain.
- Using hard-coded `:nth-child(6)` / `:nth-child(4)` selectors for adding borders to specific `.profile-menu` entries is brittle; tying these styles to semantic classes (e.g., `.primary-actions`, `.logout`) would be more robust against DOM changes.
- There are many `!important` overrides added for padding, borders, and backgrounds in the dropdown; it may be worth revisiting selector specificity or restructuring the CSS so these can be reduced to avoid future override conflicts.
## Individual Comments
### Comment 1
<location> `app/eventyay/static/pretixcontrol/scss/main.scss:744-753` </location>
<code_context>
- min-width: 200px;
+ padding: 0;
+ min-width: 192px;
#submenu {
position: absolute;
top: 0;
- right: 101%;
+ right: 100%;
font-size: 16px;
font-weight: normal;
cursor: pointer;
display: none;
- min-width: 200px;
- padding: 5px 0;
- border-radius: 4px;
+ min-width: 224px;
+ padding: 0;
+ margin: 0;
+ border-radius: 0;
z-index: 9999;
background-clip: padding-box;
- border: 1px solid rgba(0, 0, 0, 0.2);
- box-shadow: 0 5px 10px rgba(0, 0, 0, 0.2);
+ border: none;
+ box-shadow: 0 10px 15px -3px rgba(0,0,0,0.1), 0 4px 6px -4px rgba(0,0,0,0.1);
}
.profile-menu {
</code_context>
<issue_to_address>
**suggestion:** The submenu box-shadow and popover box-shadow are duplicated; consider centralizing this into a variable or mixin.
Since `.popover` and `#submenu` (and the presale stylesheet) now share this multi-layer shadow, extracting it into a shared variable or mixin (e.g. `$dropdown-shadow`) would keep elevation tweaks consistent and reduce duplication.
Suggested implementation:
```
display: none !important;
}
}
$dropdown-shadow: 0 10px 15px -3px rgba(0,0,0,0.1), 0 4px 6px -4px rgba(0,0,0,0.1);
.popover-content {
```
```
background-clip: padding-box;
border: none;
box-shadow: $dropdown-shadow;
}
```
1. Search in this file for `.popover` or other dropdown/popover elements that currently use `box-shadow: 0 10px 15px -3px rgba(0,0,0,0.1), 0 4px 6px -4px rgba(0,0,0,0.1);` and replace those with `box-shadow: $dropdown-shadow;` to fully deduplicate.
2. In the presale stylesheet (likely another SCSS file under `app/eventyay/static/`), replace the same multi-layer box-shadow with `$dropdown-shadow` as well. If that file is in a different build bundle, ensure it imports the file where `$dropdown-shadow` is defined or define the variable in a common imported partial.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
hi @mariobehling! could you please review my pr when you get a moment. i've done most of the matching manually not sure if thats the ideal approach so i'd appreciate your thoughts on it. |
mariobehling
left a comment
mariobehling
left a comment
There was a problem hiding this comment.
Thanks, please start by addressing AI reviews.
There was a problem hiding this comment.
Pull request overview
This PR standardizes dropdown menu styling across the Common Dashboard, Tickets Dashboard, and Talk interfaces to create a consistent user experience. The changes update popover containers, menu items, and separator styling.
Key Changes:
- Unified popover container appearance with consistent borders, shadows, padding, and removed arrows
- Standardized typography (14px font, 1.5 line-height), spacing (7-8px/16px padding), and icon sizing (12px) across menu items
- Replaced separator elements with border-top styling on specific menu items
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| app/eventyay/static/pretixpresale/scss/main.scss | Updated presale dropdown styling with new popover container styles, refined profile-menu spacing and typography, and replaced separators with border-top dividers |
| app/eventyay/static/pretixcontrol/scss/main.scss | Updated control interface dropdown styling with matching popover container styles, standardized profile-menu and dashboard-item spacing/typography, and refined submenu styling with consistent padding and icon sizes |
Comments suppressed due to low confidence (1)
app/eventyay/static/pretixcontrol/scss/main.scss:988
- Duplicated submenu-item styles. The
.submenu-itemblock at lines 923-952 and the one at lines 958-988 contain almost identical styling rules (a, i, and hover styles). This code duplication violates DRY principles and makes maintenance harder. Consider consolidating these into a single ruleset or using a mixin.
.submenu-item {
background-color: #fff;
padding: 7px 16px;
font-size: 12px;
line-height: 1.5;
a {
color: darken($btn-primary-bg, 10%) !important;
padding: 0 !important;
border: none !important;
background: none !important;
display: inline !important;
line-height: 1.5;
}
i {
margin-right: 7px;
width: 12px;
font-size: 12px;
vertical-align: middle;
}
&:hover {
background-color: darken($btn-primary-bg, 10%) !important;
a {
color: #fff !important;
}
}
}
}
&:hover {
#submenu {
display: block;
.submenu-item {
background-color: #fff;
padding: 8px 16px;
font-size: 12px;
line-height: 1.5;
a {
color: darken($btn-primary-bg, 10%) !important;
padding: 0 !important;
border: none !important;
background: none !important;
display: inline !important;
line-height: 1.5;
}
i {
margin-right: 8px;
width: 12px;
font-size: 12px;
display: inline-block;
vertical-align: middle;
}
&:hover {
background-color: darken($btn-primary-bg, 10%) !important;
a {
color: #fff !important;
}
}
}
💡 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.
Icons in the commons area navbar seem to be white for some reason, and no separators. Please fix that
Aqil-Ahmad
force-pushed
the
fix/dropdown
branch
from
dbd1c35 to
6bdaecd
Compare
thanks for review @Saksham-Sirohi! i noticed this icons and text issue after merging enext. i still have conflicts which i'll properly resolve soon. |
Aqil-Ahmad
force-pushed
the
fix/dropdown
branch
from
6bdaecd to
c198ad1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
app/eventyay/static/pretixcontrol/js/ui/popover.js:80
- Both JavaScript files still contain elements with the
separatorclass (lines 69 and 80 in pretixcontrol/popover.js, lines 75 and 89 in eventyay-common/popover.js). Since the CSS now hides elements with.separatorclass (viadisplay: none), these elements serve no purpose and should be removed entirely for cleaner code.
<div class="profile-menu separator"></div>
<div class="profile-menu border-top">
<a href="${basePath}${accountPath}" target="_self" class="btn btn-outline-success">
<i class="fa fa-user-circle"></i> ${window.gettext('Account')}
</a>
</div>
<div class="profile-menu admin">
<a href="${basePath}${adminPath}" target="_self" class="btn btn-outline-success">
<i class="fa fa-cog"></i> ${window.gettext('Admin')}
</a>
</div>
<div class="profile-menu separator"></div>
app/eventyay/static/eventyay-common/js/ui/popover.js:89
- Both JavaScript files still contain elements with the
separatorclass (lines 75 and 89). Since the CSS now hides elements with.separatorclass (viadisplay: none), these elements serve no purpose and should be removed entirely for cleaner code.
<div class="profile-menu separator"></div>
<div class="profile-menu border-top">
<a href="${basePath}${accountPath}" target="_self" class="btn btn-outline-success">
<i class="fa fa-user-circle"></i> ${window.gettext('Account')}
</a>
</div>
<div class="profile-menu admin">
<a href="${basePath}${adminPath}" target="_self" class="btn btn-outline-success">
<i class="fa fa-cog"></i> ${window.gettext('Admin')}
</a>
</div>
<div class="profile-menu separator"></div>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
|
Please rebase this PR. |
Aqil-Ahmad
force-pushed
the
fix/dropdown
branch
from
cd5d833 to
50e8424
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Aqil-Ahmad
force-pushed
the
fix/dropdown
branch
from
91b6312 to
d4aa8d2
Compare
Saksham-Sirohi
left a comment
Saksham-Sirohi
left a comment
There was a problem hiding this comment.
The separators are a bit different on both the dashboards to ensure parity also, try not to use !important in CSS it makes debugging difficult at a later point
I actually tested this without the |
Hi @Saksham-Sirohi! actually i tested without the !important when you requested before.. but without these, the fix doesn't work. i tried to remove the bootstrap button classes but that didn't work as well. |
|
what about the seperators not being same @Aqil-Ahmad would you like to address that |
removing the border-top class and keeping the separator divs consistently: removing the separator divs entirely: 
in both cases there is a mismatch. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sak1012
left a comment
Sak1012
left a comment
There was a problem hiding this comment.
The dropdown in talk doesn't work as expected the css looks broken, and the admin option is visible even when admin mode is not turned on.
Clicking on the user-name still points to the account centre and only clicking on the "▼" shows the dropdown.
mariobehling
left a comment
mariobehling
left a comment
There was a problem hiding this comment.
Please ensure the link "Account" is identical everywhere. Do not name it "Accounts".
Aqil-Ahmad
force-pushed
the
fix/dropdown
branch
from
b0a365c to
b0ba251
Compare
Saksham-Sirohi
left a comment
Saksham-Sirohi
left a comment
There was a problem hiding this comment.
logout button for talk area has broken UI please fix that
Aqil-Ahmad
force-pushed
the
fix/dropdown
branch
from
b0ba251 to
4a5dcee
Compare
hi @Sak1012 @Saksham-Sirohi ! |
|
hi @Saksham-Sirohi! sorry i couldn't find the issue with broken logout css before. now i have made all the requested changes. please have a look. 
|
|
It has been decided that we are getting rid of the global nav menu component altogether in #1705, so I am closing this pr in case it is needed later, we can reopen |
5 participants
the dropdown menu in the Common Dashboard and Tickets Dashboard had several inconsistencies. this pr fixes those and matches the dropdown in tickets and common to that of talk.
Summary by Sourcery
Align dashboard and profile dropdown styling across control and presale interfaces to match the Talk dropdown.
Enhancements: