General: Switch course title text color based on course color
#10375
Conversation
FelberMartin
added
ready for review
client
FelberMartin
changed the title
Feature: Switch course title text color based on course colorGeneral: Switch course title text color based on course color
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/main/webapp/app/utils/color.utils.ts (2)
20-31: Add input validation for hex color format.The function assumes valid hex color input but should handle edge cases and different formats.
Consider this enhanced implementation:
export const getColorBrightness = (color: string): number => { + // Validate hex color format + if (!/^#?([A-Fa-f0-9]{6}|[A-Fa-f0-9]{3})$/.test(color)) { + throw new Error('Invalid hex color format'); + } // Remove the hash at the start if it's there color = color.replace('#', ''); + + // Handle 3-digit hex + if (color.length === 3) { + color = color.split('').map(char => char + char).join(''); + } // Parse the r, g, b values const r = parseInt(color.substring(0, 2), 16); const g = parseInt(color.substring(2, 4), 16); const b = parseInt(color.substring(4, 6), 16); // Calculate the brightness return (r * 299 + g * 587 + b * 114) / 1000; };
47-49: Consider enhancing contrast ratio calculation for WCAG compliance.While the current implementation is functional, it could be improved to ensure WCAG 2.1 compliance by calculating actual contrast ratios.
Consider this enhanced implementation:
+const getLuminance = (r: number, g: number, b: number): number => { + const [rs, gs, bs] = [r, g, b].map(c => { + c = c / 255; + return c <= 0.03928 ? c / 12.92 : Math.pow((c + 0.055) / 1.055, 2.4); + }); + return 0.2126 * rs + 0.7152 * gs + 0.0722 * bs; +}; + +const getContrastRatio = (l1: number, l2: number): number => { + const lighter = Math.max(l1, l2); + const darker = Math.min(l1, l2); + return (lighter + 0.05) / (darker + 0.05); +}; + export const getContrastingTextColor = (color: string): string => { - return isColorDark(color) ? 'white' : 'black'; + // Remove hash and convert to RGB + const hex = color.replace('#', ''); + const r = parseInt(hex.substring(0, 2), 16); + const g = parseInt(hex.substring(2, 4), 16); + const b = parseInt(hex.substring(4, 6), 16); + + const bgLuminance = getLuminance(r, g, b); + const whiteLuminance = getLuminance(255, 255, 255); + const blackLuminance = getLuminance(0, 0, 0); + + const whiteContrast = getContrastRatio(bgLuminance, whiteLuminance); + const blackContrast = getContrastRatio(bgLuminance, blackLuminance); + + // Return color with better contrast ratio + return whiteContrast > blackContrast ? 'white' : 'black'; };src/main/webapp/app/overview/course-card-header/course-card-header.component.ts (1)
23-28: Consider using effect for reactive updates.For consistency with LtiCourseCardComponent, consider using Angular's effect for reactive updates of the title color.
- color: string; - titleColor: string; + color = signal<string>(''); + titleColor = signal<string>(''); - ngOnInit() { - this.color = this.courseColor() || this.ARTEMIS_DEFAULT_COLOR; - this.titleColor = getContrastingTextColor(this.color); - } + constructor() { + effect(() => { + this.color.set(this.courseColor() || this.ARTEMIS_DEFAULT_COLOR); + this.titleColor.set(getContrastingTextColor(this.color())); + }); + }src/main/webapp/app/course/manage/overview/course-management-card.component.ts (1)
127-132: Add error handling for invalid color values.While the color update logic is clean, it should handle potential invalid color values to prevent runtime errors.
ngOnChanges() { const targetCourseColor = this.course.color || this.ARTEMIS_DEFAULT_COLOR; if (this.courseColor !== targetCourseColor) { this.courseColor = targetCourseColor; - this.contentColor = getContrastingTextColor(this.courseColor); + try { + this.contentColor = getContrastingTextColor(this.courseColor); + } catch (error) { + console.warn(`Invalid color value: ${this.courseColor}. Falling back to white.`); + this.contentColor = '#FFFFFF'; + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/main/webapp/app/course/manage/overview/course-management-card.component.html(1 hunks)src/main/webapp/app/course/manage/overview/course-management-card.component.ts(3 hunks)src/main/webapp/app/course/manage/overview/course-management-card.scss(2 hunks)src/main/webapp/app/lti/lti-course-card.component.html(1 hunks)src/main/webapp/app/lti/lti-course-card.component.ts(2 hunks)src/main/webapp/app/overview/course-card-header/course-card-header.component.html(1 hunks)src/main/webapp/app/overview/course-card-header/course-card-header.component.ts(2 hunks)src/main/webapp/app/overview/header-course.component.html(1 hunks)src/main/webapp/app/overview/header-course.component.ts(2 hunks)src/main/webapp/app/utils/color.utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`src/main/webapp/**/*.html`: @if and @for are new and valid ...
src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/overview/course-card-header/course-card-header.component.htmlsrc/main/webapp/app/course/manage/overview/course-management-card.component.htmlsrc/main/webapp/app/lti/lti-course-card.component.htmlsrc/main/webapp/app/overview/header-course.component.html
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/lti/lti-course-card.component.tssrc/main/webapp/app/overview/course-card-header/course-card-header.component.tssrc/main/webapp/app/course/manage/overview/course-management-card.component.tssrc/main/webapp/app/utils/color.utils.tssrc/main/webapp/app/overview/header-course.component.ts
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Analyse
🔇 Additional comments (9)
src/main/webapp/app/utils/color.utils.ts (1)
38-40: LGTM! Implementation follows W3C guidelines.The function correctly determines if a color is dark using the standard brightness threshold of 128 (half of 255).
src/main/webapp/app/lti/lti-course-card.component.ts (1)
19-26: LGTM! Clean implementation with reactive updates.The component correctly uses Angular's effect for reactive updates of the contrasting text color when course color changes.
src/main/webapp/app/course/manage/overview/course-management-card.component.ts (1)
41-41: LGTM! Import and property declaration follow Angular conventions.The new utility import and property declaration adhere to Angular style guidelines with proper naming conventions.
Also applies to: 116-116
src/main/webapp/app/overview/course-card-header/course-card-header.component.html (1)
19-19: LGTM! Dynamic color styling improves accessibility.The ngStyle binding correctly implements dynamic text color for better contrast and readability.
src/main/webapp/app/lti/lti-course-card.component.html (1)
6-6: LGTM! CSS variable usage enhances maintainability.The implementation properly uses CSS variables for dynamic styling, maintaining consistency across components.
src/main/webapp/app/overview/header-course.component.html (1)
1-1: Verify color contrast meets WCAG guidelines.While the dynamic color implementation is correct, ensure that the contrast ratios meet WCAG 2.1 Level AA requirements (4.5:1 for normal text, 3:1 for large text).
#!/usr/bin/env python3 import colorsys import sys def get_contrast_ratio(color1, color2): """Calculate contrast ratio between two colors""" def get_luminance(color): # Convert hex to RGB r = int(color[1:3], 16) / 255 g = int(color[3:5], 16) / 255 b = int(color[5:7], 16) / 255 # Convert to relative luminance r = r / 12.92 if r <= 0.03928 else ((r + 0.055) / 1.055) ** 2.4 g = g / 12.92 if g <= 0.03928 else ((g + 0.055) / 1.055) ** 2.4 b = b / 12.92 if b <= 0.03928 else ((b + 0.055) / 1.055) ** 2.4 return 0.2126 * r + 0.7152 * g + 0.0722 * b l1 = get_luminance(color1) l2 = get_luminance(color2) lighter = max(l1, l2) darker = min(l1, l2) return (lighter + 0.05) / (darker + 0.05) # Test with some sample colors test_colors = [ ('#FFFFFF', '#000000'), # White on Black ('#FF0000', '#FFFFFF'), # Red on White ('#0000FF', '#FFFFFF'), # Blue on White ] for bg_color, text_color in test_colors: ratio = get_contrast_ratio(bg_color, text_color) print(f"Background: {bg_color}, Text: {text_color}") print(f"Contrast ratio: {ratio:.2f}") print(f"Meets AA for normal text (4.5:1): {ratio >= 4.5}") print(f"Meets AA for large text (3:1): {ratio >= 3.0}") print("---")Also applies to: 6-6, 9-9
src/main/webapp/app/course/manage/overview/course-management-card.scss (2)
19-19: Dynamic Text Color for Card Heading
The inline use ofcolor: var(--content-color);on line 19 correctly replaces the hardcodedwhite, allowing the text color to dynamically adjust based on the computed value. This effectively supports the improved contrast objectives outlined in the PR.
81-81: Dynamic Text Color for Links in Card Groups
By settingcolor: var(--content-color);on the anchor elements inside.card-groups, the change maintains consistency with the card heading adjustment and further ensures that any link text adapts dynamically. This supports the accessibility improvements and design flexibility.src/main/webapp/app/course/manage/overview/course-management-card.component.html (1)
2-2: Updated Inline Styling for Dynamic Text Color
The change on line 2 in thedivwith IDcourse-card-headeris well implemented. Removing thetext-whiteclass and expanding the[ngStyle]binding to include both--background-color-for-hoverand--content-colorensures that the component will now render text with a contrast that is determined dynamically by the course’s color. Please verify that thecontentColorproperty, computed in the TypeScript, reliably provides either a white or black color based on the course background’s brightness.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/webapp/app/overview/header-course.component.ts (1)
28-29: Add JSDoc comments for new properties.Consider adding documentation for the new color properties to improve code maintainability.
+ /** The background color of the course header */ public courseColor: string; + /** The contrasting text color calculated based on courseColor */ public contentColor: string;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/webapp/app/overview/header-course.component.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/overview/header-course.component.ts
🪛 GitHub Check: client-tests-selected
src/main/webapp/app/overview/header-course.component.ts
[failure] 20-20:
Class 'HeaderCourseComponent' incorrectly implements interface 'OnInit'.
[failure] 38-38:
Duplicate function implementation.
🪛 GitHub Check: client-compilation
src/main/webapp/app/overview/header-course.component.ts
[failure] 20-20:
Class 'HeaderCourseComponent' incorrectly implements interface 'OnInit'.
[failure] 38-38:
Duplicate function implementation.
🪛 GitHub Check: client-tests
src/main/webapp/app/overview/header-course.component.ts
[failure] 20-20:
Class 'HeaderCourseComponent' incorrectly implements interface 'OnInit'.
[failure] 38-38:
Duplicate function implementation.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: server-style
- GitHub Check: server-tests
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Analyse
🔇 Additional comments (1)
src/main/webapp/app/overview/header-course.component.ts (1)
1-1: LGTM! Imports are properly organized.The new imports for
OnInitandgetContrastingTextColoralign with the PR's objective of implementing dynamic text color contrast.Also applies to: 12-12
helios-aet
bot
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
helios-aet
bot
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/main/webapp/app/utils/color.utils.ts (2)
20-31: Add input validation and support for other color formats.The
getColorBrightnessfunction assumes valid hex format but lacks validation. Consider:
- Adding validation for malformed hex colors
- Supporting other color formats (rgb, hsl)
export const getColorBrightness = (color: string): number => { + // Validate hex format + if (!/^#?[0-9A-Fa-f]{6}$/.test(color)) { + throw new Error('Invalid hex color format'); + } + // Remove the hash at the start if it's there color = color.replace('#', ''); // Parse the r, g, b values const r = parseInt(color.substring(0, 2), 16); const g = parseInt(color.substring(2, 4), 16); const b = parseInt(color.substring(4, 6), 16); // Calculate the brightness return (r * 299 + g * 587 + b * 114) / 1000; };
47-49: Consider adding WCAG contrast ratio calculation.The current implementation uses a simple brightness threshold. For better accessibility, consider implementing WCAG contrast ratio calculation.
export const getContrastingTextColor = (color: string): string => { - return isColorDark(color) ? 'white' : 'black'; + // Calculate relative luminance according to WCAG 2.0 + const getLuminance = (r: number, g: number, b: number) => { + const [rs, gs, bs] = [r/255, g/255, b/255].map(c => + c <= 0.03928 ? c/12.92 : Math.pow((c + 0.055)/1.055, 2.4) + ); + return 0.2126 * rs + 0.7152 * gs + 0.0722 * bs; + }; + + const hex = color.replace('#', ''); + const r = parseInt(hex.substring(0, 2), 16); + const g = parseInt(hex.substring(2, 4), 16); + const b = parseInt(hex.substring(4, 6), 16); + + const bgLuminance = getLuminance(r, g, b); + const whiteLuminance = getLuminance(255, 255, 255); + const blackLuminance = getLuminance(0, 0, 0); + + const whiteContrast = (whiteLuminance + 0.05) / (bgLuminance + 0.05); + const blackContrast = (bgLuminance + 0.05) / (blackLuminance + 0.05); + + return whiteContrast > blackContrast ? 'white' : 'black'; };src/main/webapp/app/lti/lti-course-card.component.html (1)
6-6: Consider moving CSS variables to a shared stylesheet.While using CSS variables is a good practice, consider moving these variables to a shared stylesheet to maintain consistency across components and reduce duplication.
- [ngStyle]="{ '--background-color-for-hover': courseColor, '--content-color': contentColor }" + class="course-card-header"Create a new shared stylesheet:
.course-card-header { --background-color-for-hover: var(--course-color); --content-color: var(--content-color); }src/main/webapp/app/overview/header-course.component.html (2)
6-6: Consolidate duplicate color style bindings.Multiple elements are using the same color binding. Consider using CSS inheritance to reduce duplication.
- <h3 id="course-header-title" class="fw-medium" [class.mb-0]="!courseDescription" [ngStyle]="{ color: contentColor }"> - <h6 id="course-header-description" class="fw-normal" [ngStyle]="{ color: contentColor }"> + <div class="course-content" [ngStyle]="{ color: contentColor }"> + <h3 id="course-header-title" class="fw-medium" [class.mb-0]="!courseDescription"> + <h6 id="course-header-description" class="fw-normal"> + </div>Also applies to: 9-9
8-8: Replace inline styles with CSS classes.Using inline styles makes maintenance harder and reduces reusability.
- <div style="display: block"> + <div class="course-description-container">Add to your stylesheet:
.course-description-container { display: block; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/main/webapp/app/course/manage/overview/course-management-card.component.html(1 hunks)src/main/webapp/app/course/manage/overview/course-management-card.component.ts(3 hunks)src/main/webapp/app/course/manage/overview/course-management-card.scss(2 hunks)src/main/webapp/app/lti/lti-course-card.component.html(1 hunks)src/main/webapp/app/lti/lti-course-card.component.ts(2 hunks)src/main/webapp/app/overview/course-card-header/course-card-header.component.html(1 hunks)src/main/webapp/app/overview/course-card-header/course-card-header.component.ts(2 hunks)src/main/webapp/app/overview/header-course.component.html(1 hunks)src/main/webapp/app/overview/header-course.component.ts(2 hunks)src/main/webapp/app/utils/color.utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`src/main/webapp/**/*.html`: @if and @for are new and valid ...
src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/overview/header-course.component.htmlsrc/main/webapp/app/lti/lti-course-card.component.htmlsrc/main/webapp/app/overview/course-card-header/course-card-header.component.htmlsrc/main/webapp/app/course/manage/overview/course-management-card.component.html
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/...
src/main/webapp/app/lti/lti-course-card.component.tssrc/main/webapp/app/course/manage/overview/course-management-card.component.tssrc/main/webapp/app/overview/course-card-header/course-card-header.component.tssrc/main/webapp/app/utils/color.utils.tssrc/main/webapp/app/overview/header-course.component.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (10)
src/main/webapp/app/lti/lti-course-card.component.ts (1)
7-7: LGTM! Clean implementation using Angular signals and effects.The changes properly handle dynamic color updates using Angular's effect hook, ensuring the contrasting color is recomputed whenever the course changes.
Also applies to: 20-20, 26-26
src/main/webapp/app/overview/header-course.component.ts (2)
1-1: Remove unused OnInit interface.The class imports but doesn't implement the OnInit interface.
-import { Component, HostListener, Input, OnChanges, OnInit, inject } from '@angular/core'; +import { Component, HostListener, Input, OnChanges, inject } from '@angular/core'; -export class HeaderCourseComponent implements OnChanges, OnInit { +export class HeaderCourseComponent implements OnChanges {Also applies to: 20-20
37-39: LGTM! Proper handling of color updates.The implementation correctly updates colors in ngOnChanges, ensuring reactivity to course changes.
src/main/webapp/app/course/manage/overview/course-management-card.component.ts (2)
41-41: LGTM! Good implementation of dynamic text color.The changes correctly implement the dynamic text color adjustment by:
- Importing the utility function
- Adding the contentColor property
- Computing the contrasting color in ngOnChanges when courseColor changes
Also applies to: 115-116, 130-132
69-70:❓ Verification inconclusive
Address the TODO comment about merging courses.
The TODO comment suggests there might be redundancy in handling multiple course objects. Consider consolidating these to improve maintainability.
Let's check if we can find related course handling in the codebase:
🏁 Script executed:
#!/bin/bash # Search for similar course handling patterns rg -l "course.*: Course" src/main/webapp/app/course/Length of output: 7367
Subject: Clarify and Decide on Merging Course Handling
- File:
src/main/webapp/app/course/manage/overview/course-management-card.component.ts(Lines 69–70)- Observation: The current implementation uses a single
Courseinput, which mirrors the consistent pattern found across the course-related modules.- Recommendation:
- Revisit the TODO ("can we merge the 3 courses here?") and determine whether multiple course objects indeed exist that would benefit from consolidation into a unified model.
- If merging them simplifies the design and enhances maintainability—as might be the case if redundant course data flows are identified—proceed with the refactoring while ensuring consistency with similar components.
- Otherwise, if the single
Courseinput is intentional and aligns with the overall design, update or remove the TODO comment to avoid confusion.src/main/webapp/app/overview/course-card-header/course-card-header.component.html (1)
19-19: Verify color contrast meets WCAG guidelines.While the dynamic color adjustment is good, we should ensure the contrast ratio meets WCAG 2.1 guidelines for accessibility.
Let's create a script to check the color contrast:
#!/usr/bin/env python3 import colorsys import sys def get_relative_luminance(r, g, b): r = r / 255 if r <= 10 else ((r / 255 + 0.055) / 1.055) ** 2.4 g = g / 255 if g <= 10 else ((g / 255 + 0.055) / 1.055) ** 2.4 b = b / 255 if b <= 10 else ((b / 255 + 0.055) / 1.055) ** 2.4 return 0.2126 * r + 0.7152 * g + 0.0722 * b def get_contrast_ratio(l1, l2): lighter = max(l1, l2) darker = min(l1, l2) return (lighter + 0.05) / (darker + 0.05) # Example usage with a background color bg_color = "#007bff" # Bootstrap primary blue r = int(bg_color[1:3], 16) g = int(bg_color[3:5], 16) b = int(bg_color[5:7], 16) bg_luminance = get_relative_luminance(r, g, b) white_luminance = get_relative_luminance(255, 255, 255) black_luminance = get_relative_luminance(0, 0, 0) white_contrast = get_contrast_ratio(bg_luminance, white_luminance) black_contrast = get_contrast_ratio(bg_luminance, black_luminance) print(f"White text contrast ratio: {white_contrast:.2f}") print(f"Black text contrast ratio: {black_contrast:.2f}") print(f"WCAG AA requires: 4.5:1 for normal text, 3:1 for large text")src/main/webapp/app/course/manage/overview/course-management-card.scss (2)
18-19: Dynamic Text Color on Card Heading
The addition ofcolor: var(--content-color);on line 19 correctly enables dynamic text coloring based on the computed contrast. This helps in maintaining readability against varying background colors as intended by the PR objectives.
80-82: Consistent Link Coloring in Card Groups
Changing the anchor (a) element’s color tovar(--content-color)ensures that links within the card groups inherit the dynamic text color. This update reinforces consistent theming and accessibility across the component.src/main/webapp/app/course/manage/overview/course-management-card.component.html (2)
1-2: Dynamic CSS Variable Binding in Header
The updatedngStylebinding on the<div id="course-card-header" ...>now includes the--content-colorvariable alongside--background-color-for-hover. This change—coupled with the removal of the previously hardcoded text color—ensures that the header text color adapts correctly based on the background color. The new binding meets the accessibility requirements and adheres to the improved theming strategy.
5-15: Usage of New Angular Structural Directives (@if and @for)
The template consistently employs the new Angular syntax (using@ifand@for) in place of the classic*ngIfand*ngFor. This aligns with the current coding guidelines and improves template readability.
src/main/webapp/app/overview/course-card-header/course-card-header.component.ts
Show resolved
Hide resolved
helios-aet
bot
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
|
@coderabbitai ignore |
|
Note Reviews pausedUse the following commands to manage reviews:
|
|
@coderabbitai resolve |
Checklist
General
Client
Motivation and Context
Courses can define different course colors, that will be displayed as a background in the course cards and the header of the course details page (as an instructor). The text content placed on these colored background, always uses white color, which leads to poor contrast of the text. This decreases the readability, and violates accessibility guidelines.
Description
To solve this issue, it was decided to dynamically adjust the color of the content placed on the colored course background to be either white or black, based on the brightness of the course color.
Steps for Testing
Prerequisites:
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Code Review
Manual Tests
Screenshots
Previously
Now
A light course color leads to the text being switched to black for better contrast (left), while for darker course colors, the text color stays white (right).