Make date styles consistent across Common, Tickets, and Talk Dashboards #1446
+37
−12
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 sidebar context/date styling and hover behavior across Common, Tickets, and Talk dashboards by standardizing typography, spacing, colors, and heights, and adding shared variables for light body text and primary hover color. Flow diagram for shared styling variables across dashboardsflowchart LR
subgraph CSSVariables
var_primary["--color-primary"]
var_primary_hover["--color-primary-hover"]
var_body_color_light["$body-color-light"]
end
subgraph Dashboards
common_sidebar["Common dashboard sidebar context/date"]
tickets_sidebar["Tickets dashboard sidebar context/date"]
talk_sidebar["Talk dashboard sidebar context/date"]
end
var_primary --> var_primary_hover
var_primary --> common_sidebar
var_primary_hover --> common_sidebar
var_body_color_light --> common_sidebar
var_primary --> tickets_sidebar
var_primary_hover --> tickets_sidebar
var_body_color_light --> tickets_sidebar
var_primary --> talk_sidebar
var_body_color_light --> talk_sidebar
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 context header typography and spacing rules are now duplicated across multiple selectors (
.context-indicator, sidebar.content, and#nav-search); consider extracting a shared class or SCSS mixin so future style tweaks stay consistent in one place. - The new
--color-primary-hoverrelies oncolor-mix, which still has limited support in some environments; if legacy browsers are in scope, consider adding a fallback hover color or a precomputed static value.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The context header typography and spacing rules are now duplicated across multiple selectors (`.context-indicator`, sidebar `.content`, and `#nav-search`); consider extracting a shared class or SCSS mixin so future style tweaks stay consistent in one place.
- The new `--color-primary-hover` relies on `color-mix`, which still has limited support in some environments; if legacy browsers are in scope, consider adding a fallback hover color or a precomputed static value.
## Individual Comments
### Comment 1
<location> `app/eventyay/static/orga/css/_layout.css:328-330` </location>
<code_context>
color: var(--color-primary);
height: 56px;
+ &:hover #search-context-icon,
+ &:hover #search-context-text .context-name {
+ color: var(--color-primary-hover);
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Hover styling is applied, but focus-visible is not covered for keyboard navigation.
Keyboard users won’t get the same visual feedback, since these styles only apply on hover. Please also apply the updated color to `:focus-visible` (or at least `:focus`) on `#nav-search` so the behavior is accessible without a mouse.
Suggested implementation:
```
#nav-search {
padding: 8px 0 8px 4px;
display: flex;
flex-direction: row;
color: var(--color-primary);
height: 56px;
&:hover #search-context-icon,
&:focus-visible #search-context-icon,
&:hover #search-context-text .context-name,
&:focus-visible #search-context-text .context-name {
color: var(--color-primary-hover);
}
```
If there are existing `:hover` rules for `#search-context-icon` or `.context-name` elsewhere in this file, they can be cleaned up or aligned with this new combined `:hover` / `:focus-visible` styling to avoid duplication. Also ensure that `#nav-search` remains keyboard-focusable (e.g., it's a link/button or has appropriate `tabindex`) so the `:focus-visible` styles are actually reachable via keyboard navigation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR standardizes the date and context styling across the Common, Tickets, and Talk dashboards by unifying typography, spacing, and color variables for sidebar context headers.
Key changes:
- Replaced
opacity: 0.7with explicit color variables ($body-color-lightand--color-text-lighter) for better maintainability - Standardized font weights from
boldto600and unified line-heights to17pxacross all context headers - Introduced
--color-primary-hovervariable for consistent hover state styling
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
app/eventyay/static/pretixcontrol/scss/_sidebar.scss |
Unified context header typography and spacing in sidebar for both normal and hover states |
app/eventyay/static/pretixcontrol/scss/_sb-admin-2.scss |
Standardized context indicator styling and reduced border width from 2px to 1px |
app/eventyay/static/pretixbase/scss/_variables.scss |
Added $body-color-light variable for consistent muted text color |
app/eventyay/static/orga/css/_layout.css |
Updated search/context bar styling with consistent typography and added hover color effects |
app/eventyay/static/common/css/_variables.css |
Introduced --color-primary-hover variable for unified hover state colors |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+135
to
+136
| margin-top: 2px; | ||
| margin-bottom: 4px; |
There was a problem hiding this comment.
Inconsistent spacing for .context-name: this location uses margin-top: 2px and margin-bottom: 4px, while the same styling in _sb-admin-2.scss (lines 204-205) and _layout.css (lines 363-364) uses padding-bottom: 2px and margin-bottom: 2px. This inconsistency may result in different visual spacing across dashboards, which contradicts the PR's goal of making date styles consistent. Consider using the same spacing values across all three implementations.
Suggested change
| margin-top: 2px; | |
| margin-bottom: 4px; | |
| padding-bottom: 2px; | |
| margin-bottom: 2px; |
Comment on lines
+381
to
+382
| margin-top: 2px; | ||
| margin-bottom: 4px; |
There was a problem hiding this comment.
Inconsistent spacing for .context-name: this location uses margin-top: 2px and margin-bottom: 4px, while the same styling in _sb-admin-2.scss (lines 204-205) and _layout.css (lines 363-364) uses padding-bottom: 2px and margin-bottom: 2px. This inconsistency may result in different visual spacing across dashboards, which contradicts the PR's goal of making date styles consistent. Consider using the same spacing values across all three implementations.
Suggested change
| margin-top: 2px; | |
| margin-bottom: 4px; | |
| padding-bottom: 2px; | |
| margin-bottom: 2px; |
Comment on lines
+371
to
372
| margin-top: 1px; | ||
| } |
There was a problem hiding this comment.
Inconsistent spacing for .context-meta: this location includes margin-top: 1px, but the equivalent styling in _sidebar.scss (lines 139-142) and _sb-admin-2.scss (lines 207-210) does not include this property. For truly consistent styling across all dashboards, consider either adding this margin to all three implementations or removing it from this one.
Suggested change
| margin-top: 1px; | |
| } | |
| } | |
| } |
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.
Summary
This PR fixes the date styling on the Common Dashboard and Tickets Dashboard so it matches the Talk Dashboard.
Fixes: #1369
Untitled.video.mp4
Summary by Sourcery
Align sidebar and dashboard context/date styling across Common, Tickets, and Talk dashboards for a consistent look and feel.
Enhancements: