-
-
Notifications
You must be signed in to change notification settings - Fork 260
Fix culture-related number issues in BitColorPicker (#11920) #11924
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix culture-related number issues in BitColorPicker (#11920) #11924
Conversation
WalkthroughFixes culture-specific number formatting issues in the BitColorPicker component by introducing culture-invariant parsing for numeric inputs and CSS style generation, preventing exceptions when using non-English culture info with different decimal separators. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.razor (1)
4-9: Good culture-invariant formatting for CSS styles!The use of
FormattableString.Invarianton line 5 forthumbStylecorrectly addresses the culture-specific decimal separator issue in CSS (e.g., preventing "top: 5,5px" in cultures that use comma as decimal separator).Lines 66-68 similarly protect the alpha value bindings from culture-specific formatting.
Optional: Apply FormattableString.Invariant to line 8 for consistency
While line 8 appears to only interpolate string values (
RgbandalphaSliderBg), consider usingFormattableString.Invariantfor consistency with line 5:- var alphaSliderStyle = $"background:linear-gradient(to left,{Rgb} 0%, transparent 100%), {alphaSliderBg};"; + var alphaSliderStyle = FormattableString.Invariant($"background:linear-gradient(to left,{Rgb} 0%, transparent 100%), {alphaSliderBg};");This provides defensive consistency even though
Rgbshould already be a properly formatted string.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.razorsrc/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.razor.cs
🧰 Additional context used
🧬 Code graph analysis (1)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.razor.cs (3)
src/BlazorUI/Bit.BlazorUI/Extensions/StringExtensions.cs (1)
HasValue(5-10)src/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitInternalColor.cs (1)
Update(62-71)src/BlazorUI/Bit.BlazorUI/Components/Progress/Loading/Base/BitLoadingBase.cs (1)
Convert(163-166)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build Bit.BlazorUI
🔇 Additional comments (8)
src/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.razor.cs (6)
1-1: LGTM! Required namespace for culture-invariant parsing.The
System.Globalizationnamespace is correctly added to supportCultureInfo.InvariantCultureusage in the parsing methods.
51-73: LGTM! Simplified color type detection.The change from
StringComparison.InvariantCultureIgnoreCaseto a simpleStartsWith('#')is appropriate since '#' is an ASCII character without case variants. This simplification maintains the same behavior.
92-98: LGTM! Critical fix for culture-invariant CSS color generation.The
RgbandRgbaproperties now useFormattableString.Invariant, ensuring that the alpha channel (a double value) always uses a decimal point instead of a comma. This prevents invalid CSS like "rgba(255,0,0,0,5)" in cultures with decimal comma.
198-213: LGTM! Correct culture-invariant parsing for hue input.Line 202 correctly uses
CultureInfo.InvariantCulturewhen parsing the hue value. Since HTML range inputs always emit values with decimal points (e.g., "359.5"), this preventsFormatExceptionwhen the thread culture expects decimal commas.
215-226: LGTM! Core fix for the alpha slider FormatException.Line 219 correctly uses
CultureInfo.InvariantCulturewhen parsing the alpha value from the range input. This is the primary fix for issue #11920, preventingFormatExceptionwhen the application runs under cultures with non-invariant decimal separators.
235-247: LGTM! Correct culture-invariant formatting for ARIA labels.The ARIA label construction properly handles culture formatting:
- Line 237: RGB values are integers, so no culture issue
- Line 241: Alpha percentage correctly uses
FormattableString.Invariantto ensure consistent formatting (e.g., "50%" instead of "50,0%" in cultures with decimal comma)The ARIA labels will now be consistent and readable across all cultures.
src/BlazorUI/Bit.BlazorUI/Components/Inputs/ColorPicker/BitColorPicker.razor (2)
31-31: LGTM!Clean usage of the centralized
thumbStylevariable with culture-invariant formatting.
54-70: Culture-invariant parsing is correctly implemented.The alpha and hue input handlers properly use culture-invariant parsing:
HandleOnAlphaInput(line 219):Convert.ToDouble(args.Value, CultureInfo.InvariantCulture)HandleOnHueInput(line 202):Convert.ToDouble(args.Value, CultureInfo.InvariantCulture)Combined with the
FormattableString.Invariantformatting in the HTML output bindings (lines 66-68), this ensures numeric values are parsed and displayed consistently regardless of the thread's culture settings, resolving the FormatException issue described in the PR objectives.
closes #11920
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.