Feat/color schemes rework#3878
Conversation
|
@tpurschke Ich habe die update sql erstmal 9.1.1.sql genannt, ich bin mir da nicht sicher in welche version und datei die kommt |
There was a problem hiding this comment.
Pull Request Overview
This pull request adds a customizable color scheme feature to the UI, allowing users to select from predefined color schemes (blue, green, red, purple). The changes enable dynamic theming of the navigation bar and headings through CSS variables.
- Introduced
ColorSchemeclass with predefined color schemes and properties for gradient colors - Added color scheme selection UI in settings with persistence to configuration
- Updated CSS to use CSS variables for dynamic color theming
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| roles/lib/files/FWO.Config.Api/Data/ColorScheme.cs | New class defining available color schemes with hex values and a lookup method |
| roles/lib/files/FWO.Config.Api/Data/ConfigData.cs | Added ColorScheme property to persist user's color scheme selection |
| roles/ui/files/FWO.UI/Services/ColorThemeService.cs | New service for managing user color mode state (appears unused in this PR) |
| roles/ui/files/FWO.UI/Shared/NavigationMenu.razor | Applied color scheme CSS variables to navigation bar for dynamic theming |
| roles/ui/files/FWO.UI/Pages/Settings/SettingsDefaults.razor | Added UI dropdown for color scheme selection with save functionality |
| roles/ui/files/FWO.UI/wwwroot/css/site.css | Updated gradient and heading styles to support CSS variables with fallbacks |
| roles/database/files/upgrade/8.8.11.sql | Database migration adding localized text entries for color schemes |
| roles/database/files/sql/idempotent/fworch-texts.sql | Added localized text entries for color scheme UI labels |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| INSERT INTO txt VALUES ('color_scheme_standard', 'German', 'Standard Farbschema'); | ||
| INSERT INTO txt VALUES ('color_scheme_standard', 'English', 'Standard Color Scheme'); |
There was a problem hiding this comment.
The database migration includes text entries for 'color_scheme_standard', but this color scheme name doesn't exist in the ColorScheme.AvailableSchemes list. The actual default scheme is named 'color_scheme_blue'. Either add 'color_scheme_standard' as an available scheme or remove these unused text entries.
| INSERT INTO txt VALUES ('color_scheme_standard', 'German', 'Standard Farbschema'); | |
| INSERT INTO txt VALUES ('color_scheme_standard', 'English', 'Standard Color Scheme'); |
| { | ||
| configData.DefaultLanguage = selectedLanguage.Name; | ||
| configData.ColorScheme = selectedColorScheme.Name; | ||
| StateHasChanged(); |
There was a problem hiding this comment.
Calling StateHasChanged() in the middle of the Save() method is unnecessary and could cause premature re-rendering before the save operation completes. If a state refresh is needed, it should be called after all data modifications are complete, or removed entirely if the framework handles it automatically after the async operation completes.
| StateHasChanged(); |
| bool allGood = false; | ||
| if (_UserColorModes.TryAdd(key, mode)) | ||
| { | ||
| allGood = true; | ||
|
|
||
| } | ||
| else | ||
| { | ||
| allGood = _UserColorModes.TryUpdate(key, mode, _UserColorModes[key]); | ||
| } |
There was a problem hiding this comment.
Both branches of this 'if' statement write to the same variable - consider using '?' to express intent better.
| bool allGood = false; | |
| if (_UserColorModes.TryAdd(key, mode)) | |
| { | |
| allGood = true; | |
| } | |
| else | |
| { | |
| allGood = _UserColorModes.TryUpdate(key, mode, _UserColorModes[key]); | |
| } | |
| bool allGood = _UserColorModes.TryAdd(key, mode) | |
| ? true | |
| : _UserColorModes.TryUpdate(key, mode, _UserColorModes[key]); |
There was a problem hiding this comment.
all changes to txt table are made only in roles/database/files/sql/idempotent/fworch-texts.sql
I do not really see any file with that name but we do not need it anyway. Also the file 8.8.11 should be removed |
|
| <div class="position-sticky" style="z-index:15; top:0px;"> | ||
| <nav id="navbar" class="navbar navbar-expand-xl navbar-dark bg-blue shadow w-100"> | ||
| <nav id="navbar" class="navbar navbar-expand-xl navbar-dark bg-blue shadow w-100" | ||
| style="--bg-color:@(ColorScheme.GetSchemeByName(userConfig.ColorScheme).Hex); |
There was a problem hiding this comment.
This looks evil to me. Using styles parameter for this is ok if not possible otherwise but using the parameter to set variable values is odd to me. Can you explain why this is needed instead of just using the values of ColorScheme.GetSchemeByName(userConfig.ColorScheme).Hex directly in the styles?
Merging into Importer Rework branch