Align Talk component UI with Tickets page #1467
Open
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
Contributor
Reviewer's GuideAligns the Talk component and Tickets page headers by unifying container widths, removing card shadows, standardizing logo styling, and replacing inline locale links with dropdowns that are horizontally aligned with the user menu across views. Flow diagram for unified header layout between Tickets and Talk componentsflowchart LR
Header["Header container (top-bg / page-header-links)"]
HeaderLeft["header-part.header-left\n(Organizer back link, optional)"]
HeaderCenter["Event logo (event-logo)\nBorder-radius: 0"]
HeaderRight["header-part.header-right\nAligned controls"]
HeaderRightLocales["Locales dropdown (details.locales)\nAlways dropdown, no inline links"]
HeaderRightUser["User menu dropdown\n(fragment_login_status / user dropdown)"]
MainContainer["Main container (main-container / container-fluid)\nmax-width: 1200px, unified padding"]
MainCard["Main card (main-card / main-box)\nNo box-shadow, border:none"]
Header --> HeaderLeft
Header --> HeaderCenter
Header --> HeaderRight
HeaderRight --> HeaderRightLocales
HeaderRight --> HeaderRightUser
Header --> MainContainer
MainContainer --> MainCard
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:
- The layout adjustments for
.page-header-links/.header-partare duplicated acrosscustom.css,custom.scss, and_theme.scss; consider consolidating these rules into a single SCSS source to avoid divergence and simplify future maintenance. - Several style changes rely on
!important(e.g. logoborder-radius,#main-cardbox-shadow) or commented‑out properties; it may be cleaner to remove or refactor the underlying rules instead of overriding them with!importantor leaving dead code. - The new
<details>-based language dropdown usesrole="menu"/role="menuitem"but otherwise relies on native<details>behavior; you may want to either remove the ARIA menu roles and let the native disclosure semantics stand, or add the appropriate ARIA attributes and keyboard handling to fully match the menu pattern.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The layout adjustments for `.page-header-links` / `.header-part` are duplicated across `custom.css`, `custom.scss`, and `_theme.scss`; consider consolidating these rules into a single SCSS source to avoid divergence and simplify future maintenance.
- Several style changes rely on `!important` (e.g. logo `border-radius`, `#main-card` box-shadow) or commented‑out properties; it may be cleaner to remove or refactor the underlying rules instead of overriding them with `!important` or leaving dead code.
- The new `<details>`-based language dropdown uses `role="menu"` / `role="menuitem"` but otherwise relies on native `<details>` behavior; you may want to either remove the ARIA menu roles and let the native disclosure semantics stand, or add the appropriate ARIA attributes and keyboard handling to fully match the menu pattern.
## Individual Comments
### Comment 1
<location> `app/eventyay/static/cfp/css/_layout.css:195-202` </location>
<code_context>
- min-height: 300px;
- box-shadow: 0 5px 10px rgb(0 0 0 / 0.2);
- padding: 0;
+ min-height: 470.4px;
+ margin-bottom: 10px;
+ padding-left:15px;
+ padding-right: 15px;
+ margin-left: 0px;
+ margin-right: 0px;
+ line-height: 20px;
+ text-size-adjust: 100%;
+
+
</code_context>
<issue_to_address>
**suggestion:** Fixed `min-height: 470.4px` on `#main-card` is a brittle, device‑specific value that may cause layout issues on smaller screens.
This value is likely tuned to a specific viewport and can introduce scrollbars or large empty space on other screen sizes. Consider a more responsive approach (e.g., percentage-based height, content-driven `min-height`, or a rounded `rem`/`px` value) so it scales cleanly across breakpoints.
Suggested implementation:
```
min-height: 30rem;
```
If `#main-card` still appears too tall on smaller screens, consider adding a responsive constraint such as `min-height: min(30rem, 60vh);` (if your browser support matrix allows) or overriding `min-height` within existing mobile breakpoints/media queries to reduce it on narrow viewports.
</issue_to_address>
### Comment 2
<location> `app/eventyay/static/pretixpresale/scss/main.scss:54-55` </location>
<code_context>
.locales{
align-content: center;
}
+.locales summary {
+ color: white !important;
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Forcing `.locales summary` text to white with `!important` may hurt contrast on light header backgrounds and is hard to override.
Because this selector is broad and uses `!important`, it will style all locale dropdown summaries, including on light headers where white text may become unreadable and block theme overrides. Please scope this color to only the dark-header variant(s) or use a more specific selector without `!important` so theme styles can override it where needed.
Suggested implementation:
```
.header-dark .locales summary {
color: #fff;
}
```
1. Replace `.header-dark` with the actual dark-header or dark-theme class used in your layout (e.g. `.navbar-dark`, `.topbar--dark`, etc.).
2. If there are multiple dark header variants, you may want to duplicate or extend this rule for each relevant parent selector (e.g. `.header-dark .locales summary, .navbar-dark .locales summary { ... }`).
3. If light themes should define their own summary color explicitly, ensure corresponding selectors exist elsewhere in your SCSS to set a suitable contrasting color.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
195
to
202
| min-height: 470.4px; | ||
| margin-bottom: 10px; | ||
| padding-left:15px; | ||
| padding-right: 15px; | ||
| margin-left: 0px; | ||
| margin-right: 0px; | ||
| line-height: 20px; | ||
| text-size-adjust: 100%; |
Contributor
There was a problem hiding this comment.
suggestion: Fixed min-height: 470.4px on #main-card is a brittle, device‑specific value that may cause layout issues on smaller screens.
This value is likely tuned to a specific viewport and can introduce scrollbars or large empty space on other screen sizes. Consider a more responsive approach (e.g., percentage-based height, content-driven min-height, or a rounded rem/px value) so it scales cleanly across breakpoints.
Suggested implementation:
min-height: 30rem;
If #main-card still appears too tall on smaller screens, consider adding a responsive constraint such as min-height: min(30rem, 60vh); (if your browser support matrix allows) or overriding min-height within existing mobile breakpoints/media queries to reduce it on narrow viewports.
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request aligns the Talk component UI with the Tickets page to achieve visual consistency across different sections of the application. The changes focus on standardizing the layout, removing visual elements like box shadows and logo corner rounding, and replacing inline language selectors with a unified dropdown approach.
Key Changes:
- Unified content container width across components (max-width: 1200px) and removed box shadows for a flatter design
- Replaced inline language links with a consistent dropdown implementation across all components
- Removed automatic corner rounding from logo images (border-radius: 0) across event pages and Talk component
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 23 comments.
Show a summary per file
| File | Description |
|---|---|
| app/eventyay/static/pretixpresale/scss/main.scss | Commented out box-shadow on .main-box and added white color styling for locale dropdown summary |
| app/eventyay/static/pretixpresale/scss/custom.scss | Added flexbox layout properties for page header alignment and spacing |
| app/eventyay/static/pretixpresale/scss/custom.css | Converted nested SCSS styles to flat CSS with flexbox layout for headers |
| app/eventyay/static/pretixpresale/scss/_theme.scss | Added duplicate flexbox layout rules for page header alignment |
| app/eventyay/static/pretixpresale/scss/_event.scss | Changed logo border-radius from 10px to 0 for both desktop and mobile views |
| app/eventyay/static/cfp/css/_layout.css | Extensive changes including language dropdown styling, container width adjustment (1200px), min-height specification, and removal of box shadows |
| app/eventyay/presale/templates/pretixpresale/event/base.html | Replaced inline language links with dropdown implementation in two conditional sections, changed container to container-fluid |
| app/eventyay/common/templates/common/base.html | Removed conditional logic for inline vs dropdown language selector, now always uses dropdown for multiple languages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+115
to
+122
| .page-header-links { | ||
| display: flex; | ||
| justify-content: space-between; | ||
| align-items: center; | ||
| flex-wrap: wrap; | ||
| padding-left: 1rem; | ||
| padding-right: 1rem; | ||
| } |
There was a problem hiding this comment.
Duplicate style definition detected. The .page-header-links selector is defined twice in this file (lines 108-114 and 115-122), with the second definition overriding the first. The first definition (lines 108-114) only contains nested styles for link colors, while the second adds layout properties. These should be merged into a single block to avoid confusion and maintain cleaner code.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Changes made:
Screenshots:
Before:
After:
Before:
After:
Before:
After:
Linked Issue:
Fixes #1365
Summary by Sourcery
Align Talk component layout and event pages with the Tickets page for consistent header, container, and language selector behavior.
New Features:
Enhancements: