chore(application-system): Refactoring slider form field #17481
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
warning [email protected]: This version is no longer supported. Please see https://eslint.org/version-support for other options. WalkthroughThis pull request introduces enhancements to the Slider component across multiple files in the application's libraries. The changes primarily focus on adding new customization properties ( Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17481 +/- ##
==========================================
- Coverage 35.61% 35.60% -0.02%
==========================================
Files 7015 7015
Lines 150332 150380 +48
Branches 42978 43005 +27
==========================================
+ Hits 53541 53542 +1
- Misses 96791 96838 +47 Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 10 Total Test Services: 0 Failed, 10 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (3) |
HjorturJ
changed the title
…s into chore/slider-refactor
…s into chore/slider-refactor
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
libs/application/ui-components/src/components/Slider/Slider.css.ts (1)
1-1: Remove unused import.The
styleVariantsimport is not used in this file.-import { keyframes, style, styleVariants } from '@vanilla-extract/css' +import { keyframes, style } from '@vanilla-extract/css'libs/application/templates/reference-template/src/forms/exampleForm/simpleInputsSection/sliderSubsection.ts (2)
203-243: Consider using dynamic dates.The hardcoded dates for 2024 might become outdated. Consider using relative dates based on the current date.
- date: new Date('2024-01-06'), + date: new Date(new Date().setDate(new Date().getDate() + 6)), - date: new Date('2024-01-12'), + date: new Date(new Date().setDate(new Date().getDate() + 12)),
15-252: Consider grouping similar examples.The examples could be organized better by grouping similar configurations together (e.g., all tooltip examples, all color examples, etc.).
libs/application/ui-components/src/components/Slider/Slider.tsx (2)
190-192: Use optional chaining for label validation.The label validation could be simplified using optional chaining.
- return label && label.singular && label.plural + return label?.singular && label?.plural🧰 Tools
🪛 Biome (1.9.4)
[error] 191-191: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
206-212: Consider enhancing date formatting.The date formatting could be more robust by:
- Adding error handling for invalid dates
- Supporting more locale options
const formatDates = (date: string | Date | undefined): string => { if (date === undefined) return '' + try { if (typeof date === 'string') return date - return date.toLocaleDateString('is-IS') + return date.toLocaleDateString('is-IS', { + year: 'numeric', + month: '2-digit', + day: '2-digit' + }) + } catch (error) { + console.error('Invalid date:', error) + return '' + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
libs/application/core/src/lib/fieldBuilders.ts(2 hunks)libs/application/templates/parental-leave/src/fields/Duration/Duration.tsx(1 hunks)libs/application/templates/reference-template/src/forms/exampleForm/simpleInputsSection/sliderSubsection.ts(1 hunks)libs/application/types/src/lib/Fields.ts(2 hunks)libs/application/ui-components/src/components/Slider/Slider.css.ts(5 hunks)libs/application/ui-components/src/components/Slider/Slider.tsx(9 hunks)libs/application/ui-fields/src/lib/SliderFormField/SliderFormField.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/application/templates/parental-leave/src/fields/Duration/Duration.tsx
🧰 Additional context used
📓 Path-based instructions (6)
libs/application/core/src/lib/fieldBuilders.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/ui-fields/src/lib/SliderFormField/SliderFormField.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/types/src/lib/Fields.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/templates/reference-template/src/forms/exampleForm/simpleInputsSection/sliderSubsection.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/ui-components/src/components/Slider/Slider.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/ui-components/src/components/Slider/Slider.css.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🪛 Biome (1.9.4)
libs/application/ui-components/src/components/Slider/Slider.tsx
[error] 191-191: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: tests (judicial-system-backend)
- GitHub Check: build (web)
- GitHub Check: tests (judicial-system-backend)
- GitHub Check: build (web)
- GitHub Check: tests (judicial-system-backend)
- GitHub Check: build (web)
- GitHub Check: tests (judicial-system-backend)
- GitHub Check: build (web)
- GitHub Check: tests (judicial-system-backend)
- GitHub Check: build (web)
- GitHub Check: tests (judicial-system-backend)
- GitHub Check: build (web)
- GitHub Check: tests (judicial-system-backend)
- GitHub Check: build (web)
- GitHub Check: tests (judicial-system-backend)
- GitHub Check: build (web)
- GitHub Check: tests (judicial-system-backend)
- GitHub Check: build (web)
- GitHub Check: tests (judicial-system-backend)
- GitHub Check: build (web)
🔇 Additional comments (12)
libs/application/ui-components/src/components/Slider/Slider.css.ts (2)
29-29: LGTM! Good use of theme color.Using
theme.color.blue400instead of hardcoded hex value improves consistency and maintainability.
46-46: LGTM! Good use of CSS variables.Using CSS variables for thumb color enables dynamic theming and improves maintainability.
Also applies to: 66-66, 77-77
libs/application/ui-fields/src/lib/SliderFormField/SliderFormField.tsx (3)
41-42: LGTM! Good addition of color customization props.The new props
textColorandprogressOverlayColoralign with the PR objectives for enhanced customization.
91-100: LGTM! Safer label handling.The conditional check for label existence prevents potential runtime errors.
111-112: LGTM! Proper prop forwarding.The new color props are correctly forwarded to the Slider component.
libs/application/ui-components/src/components/Slider/Slider.tsx (2)
37-51: LGTM! Well-structured color handling.The
ColorValuesinterface andgetColorValuesfunction provide a robust way to handle both hex and theme colors.
219-222: LGTM! Improved tooltip positioning.The tooltip positioning now correctly handles margin adjustments based on label presence.
libs/application/types/src/lib/Fields.ts (4)
766-766: LGTM: Making calculateCellStyle optional improves flexibility.This change allows for simpler slider configurations when custom cell styling is not needed.
772-775: LGTM: Making label optional enhances component flexibility.This change allows for non-labeled sliders, which is useful for cases where labels are not required or are handled externally.
778-784: LGTM: Enhanced type flexibility for date fields.Supporting both string and Date types for rangeDates improves type flexibility and makes the interface more developer-friendly.
792-793: LGTM: New color customization properties.The addition of
textColorandprogressOverlayColorproperties enhances the slider's customization capabilities, aligning with the PR's objective to improve visual customization.libs/application/core/src/lib/fieldBuilders.ts (1)
995-996: LGTM: Builder implementation aligns with interface changes.The buildSliderField function correctly implements the new color customization properties, maintaining consistency with the SliderField interface.
Also applies to: 1026-1027
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libs/application/ui-components/src/components/Slider/Slider.tsx (2)
168-204: Validation functions could be more DRY.The validation functions have similar patterns and could be refactored to reduce duplication.
Consider consolidating the validation logic:
-const hasAllRequiredForMinMaxLabels = () => { - if (showMinMaxLabels) { - if (hasValidLabels()) return true - throw new Error( - 'The labels object has to be defined if you want to show the min max labels', - ) - } - return false -} - -const hasAllRequiredForTooltip = () => { - if (showToolTip) { - if (hasValidLabels()) return true - throw new Error( - 'The labels object has to be defined if you want to show the tooltip', - ) - } - return false -} - -const hasValidLabels = () => { - return label && label.singular && label.plural -} - -const hasAllRequiredForLabels = () => { - if (showLabel) { - if (hasValidLabels()) return true - throw new Error( - 'The labels object has to be defined if you want to show them', - ) - } - return false -} +const hasValidLabels = () => label?.singular && label?.plural + +const validateLabels = (showFeature: boolean, featureName: string) => { + if (showFeature) { + if (hasValidLabels()) return true + throw new Error( + `The labels object has to be defined if you want to show ${featureName}`, + ) + } + return false +} + +const hasAllRequiredForMinMaxLabels = () => validateLabels(showMinMaxLabels, 'min max labels') +const hasAllRequiredForTooltip = () => validateLabels(showToolTip, 'tooltip') +const hasAllRequiredForLabels = () => validateLabels(showLabel, 'labels')🧰 Tools
🪛 Biome (1.9.4)
[error] 191-191: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
191-191: Use optional chaining for safer property access.The current code could benefit from optional chaining to make it more robust.
-return label && label.singular && label.plural +return label?.singular && label?.plural🧰 Tools
🪛 Biome (1.9.4)
[error] 191-191: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
libs/application/core/src/lib/fieldBuilders.ts(2 hunks)libs/application/templates/parental-leave/src/fields/Duration/Duration.tsx(1 hunks)libs/application/templates/reference-template/src/forms/exampleForm/simpleInputsSection/sliderSubsection.ts(1 hunks)libs/application/types/src/lib/Fields.ts(2 hunks)libs/application/ui-components/src/components/Slider/Slider.css.ts(5 hunks)libs/application/ui-components/src/components/Slider/Slider.tsx(9 hunks)libs/application/ui-fields/src/lib/SliderFormField/SliderFormField.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/application/templates/parental-leave/src/fields/Duration/Duration.tsx
🧰 Additional context used
📓 Path-based instructions (6)
libs/application/ui-fields/src/lib/SliderFormField/SliderFormField.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/types/src/lib/Fields.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/ui-components/src/components/Slider/Slider.css.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/core/src/lib/fieldBuilders.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/ui-components/src/components/Slider/Slider.tsx (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/templates/reference-template/src/forms/exampleForm/simpleInputsSection/sliderSubsection.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🪛 Biome (1.9.4)
libs/application/ui-components/src/components/Slider/Slider.tsx
[error] 191-191: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: tests (judicial-system-backend)
- GitHub Check: tests (judicial-system-backend)
- GitHub Check: tests (judicial-system-backend)
- GitHub Check: tests (judicial-system-backend)
- GitHub Check: tests (judicial-system-backend)
- GitHub Check: tests (judicial-system-backend)
- GitHub Check: tests (judicial-system-backend)
- GitHub Check: tests (judicial-system-backend)
- GitHub Check: tests (judicial-system-backend)
- GitHub Check: tests (judicial-system-backend)
🔇 Additional comments (17)
libs/application/ui-components/src/components/Slider/Slider.css.ts (3)
1-1: LGTM! Added styleVariants import.The addition of
styleVariantsimport from@vanilla-extract/cssaligns with the goal of enhancing styling capabilities.
29-29: Good improvement! Using theme colors instead of hardcoded values.Replacing hardcoded hex value
#0061ffwiththeme.color.blue400improves maintainability and consistency.
46-46: Nice enhancement! Using CSS variables for dynamic styling.Replacing hardcoded hex values with CSS variable
var(--thumb-color, #00e4ca)enables runtime customization of the thumb color while maintaining a fallback.Also applies to: 66-66, 77-77
libs/application/ui-fields/src/lib/SliderFormField/SliderFormField.tsx (3)
41-42: LGTM! Added color customization props.The addition of
textColorandprogressOverlayColorprops enhances the slider's customization capabilities.
91-100: Good improvement! More robust label handling.The updated label handling with the
&&operator prevents accessing properties of undefined, making the code more robust.
111-112: LGTM! Passing color props to Slider component.Correctly passing the new color customization props to the underlying Slider component.
libs/application/templates/reference-template/src/forms/exampleForm/simpleInputsSection/sliderSubsection.ts (4)
15-24: LGTM! Basic slider example.Good start with a minimal slider configuration.
50-50: Good practice! Using theme colors consistently.Consistently using theme colors (
theme.color.black,theme.color.red600,theme.color.blue200) instead of hardcoded values.Also applies to: 102-102, 247-247
235-243: LGTM! Well-structured date range configuration.The date range configuration is well-structured with clear start and end dates, including descriptive messages.
221-224: Good practice! Setting default and current values.Setting both
defaultValueandcurrentIndexensures consistent initial state.libs/application/ui-components/src/components/Slider/Slider.tsx (2)
37-51: LGTM! Well-structured color handling.The
ColorValuesinterface andgetColorValuesfunction provide a robust way to handle both hex and theme colors.
206-212: LGTM! Flexible date handling.The
formatDatesfunction handles both string and Date types, making it more flexible.libs/application/types/src/lib/Fields.ts (4)
766-766: LGTM! MakingcalculateCellStyleoptional improves flexibility.The change maintains backward compatibility while allowing simpler slider configurations that don't require custom cell styling.
772-775: LGTM! Makinglabeloptional is a good improvement.The change maintains backward compatibility while allowing sliders without labels.
778-778: LGTM! Supporting both string and Date types provides more flexibility.The change allows direct usage of Date objects while maintaining compatibility with string dates.
Also applies to: 782-782
792-793: LGTM! New color customization options align with PR objectives.The addition of optional
textColorandprogressOverlayColorproperties enhances the slider's customization capabilities while maintaining backward compatibility.libs/application/core/src/lib/fieldBuilders.ts (1)
995-996: LGTM! Builder function correctly handles new color customization options.The changes properly extract and pass through the new optional properties while maintaining the existing patterns used throughout the file.
Also applies to: 1026-1027
Refactoring slider for field to add more customization and improving layout
Task link
What
Trying to improve the slider layout so it doesnt overlap other field elements by default or its own labels, as well as adding more properties to customize various elements, specifically color of most elements.
Why
Makes it much easier and appealing to use the slider field to other applications.
Screenshots / Gifs
Example application slider form section updated:
Before and after for parental leave application sliders (same look, slightly different margins for overlap issues):
...
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Enhancements
These updates provide more flexibility and customization options for slider components across the application.