-
Notifications
You must be signed in to change notification settings - Fork 47
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
Numeric range field added (#799) #800
Conversation
Added a custom field to handle numeric ranges. The field checks for the first key in the properties of the schema and will add min and max to it when the values are put in.
WalkthroughThe Changes
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 X ? TipsChat with CodeRabbit Bot (
|
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.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- spiffworkflow-frontend/package-lock.json
Files selected for processing (3)
- spiffworkflow-frontend/src/components/CustomForm.tsx (3 hunks)
- spiffworkflow-frontend/src/index.css (24 hunks)
- spiffworkflow-frontend/src/rjsf/custom_widgets/NumericRangeField/NumericRangeField.tsx (1 hunks)
Files skipped from review due to trivial changes (1)
- spiffworkflow-frontend/src/index.css
Additional comments: 5
spiffworkflow-frontend/src/components/CustomForm.tsx (4)
8-9: The import statements for
NumericRangeField
andRegistryFieldsType
are correctly added to support the new numeric range field functionality.41-43: The
fields
object with thenumeric_range
property is correctly set toNumericRangeField
, which is necessary for the new feature.273-273: The
fields
prop is correctly passed to theForm
component, ensuring theNumericRangeField
is available for use in forms.38-47: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [5-276]
No further issues or improvements are identified in the rest of the file. The existing code remains unchanged and the new feature integration appears to be isolated and well-contained.
spiffworkflow-frontend/src/rjsf/custom_widgets/NumericRangeField/NumericRangeField.tsx (1)
- 51-53: The logic to determine
identifierToUse
assumes thatschema.properties
will always have at least one key. Verify that this assumption holds true in all cases whereNumericRangeField
is used, or consider adding a fallback or error handling for cases whereschema.properties
might be empty or undefined.
spiffworkflow-frontend/src/rjsf/custom_widgets/NumericRangeField/NumericRangeField.tsx
Outdated
Show resolved
Hide resolved
spiffworkflow-frontend/src/rjsf/custom_widgets/NumericRangeField/NumericRangeField.tsx
Outdated
Show resolved
Hide resolved
spiffworkflow-frontend/src/rjsf/custom_widgets/NumericRangeField/NumericRangeField.tsx
Outdated
Show resolved
Hide resolved
spiffworkflow-frontend/src/rjsf/custom_widgets/NumericRangeField/NumericRangeField.tsx
Outdated
Show resolved
Hide resolved
spiffworkflow-frontend/src/rjsf/custom_widgets/NumericRangeField/NumericRangeField.tsx
Outdated
Show resolved
Hide resolved
spiffworkflow-frontend/src/rjsf/custom_widgets/NumericRangeField/NumericRangeField.tsx
Outdated
Show resolved
Hide resolved
please run |
@Kayvon-Martinez can you supply a sample process model or otherwise describe or give examples of what the json schema and ui schema should look like? we were having some issues figuring out how to use it and it was giving us validation errors when submitting the form. when you have some time, feel free to hit us up as well and we can go over it together. |
Hi @burnettk. I will attach examples of the schema and uischema below. I am also about to upload a test process and will link that when I do. |
…ne with current conventions w/ burnettk
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.
Review Status
Actionable comments generated: 9
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- spiffworkflow-frontend/package-lock.json
Files selected for processing (5)
- spiffworkflow-frontend/src/components/CustomForm.tsx (5 hunks)
- spiffworkflow-frontend/src/index.css (23 hunks)
- spiffworkflow-frontend/src/rjsf/carbon_theme/index.css (1 hunks)
- spiffworkflow-frontend/src/rjsf/custom_widgets/NumericRangeField/NumericRangeField.tsx (1 hunks)
- spiffworkflow-frontend/src/routes/TaskShow.tsx (1 hunks)
Files skipped from review due to trivial changes (2)
- spiffworkflow-frontend/src/index.css
- spiffworkflow-frontend/src/rjsf/carbon_theme/index.css
Additional comments: 4
spiffworkflow-frontend/src/components/CustomForm.tsx (1)
- 297-297: The addition of the
fields
prop to theForm
component is in line with the PR objectives to integrate theNumericRangeField
.spiffworkflow-frontend/src/routes/TaskShow.tsx (3)
204-226: The PR overview does not mention changes to exported or public entities, and the hunk does not show any such changes. Confirm that this aligns with the PR's objectives and that no public API changes are required for the new
NumericRangeField
component.226-226: Error handling is still in place for the
handleSignalSubmit
function, which is good practice. Ensure that all other necessary error handling is also maintained throughout the codebase.204-226: The verification scripts have confirmed the presence of the
NumericRangeField.tsx
file within thespiffworkflow-frontend/src/rjsf/custom_widgets/NumericRangeField
directory. However, there were no matches found forNumericRangeField
in eitherindex.css
orcarbon_theme/index.css
, which suggests that the CSS changes might not be directly related to theNumericRangeField
by name or they might be using a different class or ID for styling.Given this information, I can confirm that the
NumericRangeField
component has been added to the codebase as intended by the PR. The absence of direct references toNumericRangeField
in the CSS files does not necessarily indicate an issue, as the CSS changes could be using different selectors or could be more general and not specific to theNumericRangeField
.Analysis chain
The hunk does not show the addition of the
NumericRangeField
component or CSS changes. Verify that these changes are present elsewhere in the codebase and that they align with the PR's objectives.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the NumericRangeField component has been added fd 'NumericRangeField.tsx' --exec grep -Hn 'NumericRangeField' # Verify that CSS changes have been made fd 'index.css' --exec grep -Hn 'NumericRangeField' fd 'carbon_theme/index.css' --exec grep -Hn 'NumericRangeField'Length of output: 307
spiffworkflow-frontend/src/rjsf/custom_widgets/NumericRangeField/NumericRangeField.tsx
Outdated
Show resolved
Hide resolved
spiffworkflow-frontend/src/rjsf/custom_widgets/NumericRangeField/NumericRangeField.tsx
Outdated
Show resolved
Hide resolved
spiffworkflow-frontend/src/rjsf/custom_widgets/NumericRangeField/NumericRangeField.tsx
Outdated
Show resolved
Hide resolved
spiffworkflow-frontend/src/rjsf/custom_widgets/NumericRangeField/NumericRangeField.tsx
Show resolved
Hide resolved
spiffworkflow-frontend/src/rjsf/custom_widgets/NumericRangeField/NumericRangeField.tsx
Outdated
Show resolved
Hide resolved
spiffworkflow-frontend/src/rjsf/custom_widgets/NumericRangeField/NumericRangeField.tsx
Outdated
Show resolved
Hide resolved
…is greater than min w/ burnettk
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- spiffworkflow-frontend/src/components/CustomForm.tsx (5 hunks)
- spiffworkflow-frontend/src/rjsf/carbon_theme/index.css (1 hunks)
- spiffworkflow-frontend/src/rjsf/custom_widgets/NumericRangeField/NumericRangeField.tsx (1 hunks)
Files skipped from review due to trivial changes (2)
- spiffworkflow-frontend/src/rjsf/carbon_theme/index.css
- spiffworkflow-frontend/src/rjsf/custom_widgets/NumericRangeField/NumericRangeField.tsx
Files skipped from review as they are similar to previous changes (1)
- spiffworkflow-frontend/src/components/CustomForm.tsx
@coderabbitai hey, can you write some documentation about how this feature works and how to use it (how to write json files and whatnot) in markdown format? |
# Numeric Range Field Documentation
## Overview
The `NumericRangeField` component is a new feature in the `spiffworkflow-frontend` that allows users to input numeric ranges. This component is designed to work with JSON schemas and provides two text inputs for users to enter minimum and maximum values for a given numeric range.
## Integration into Forms
To integrate the `NumericRangeField` into a form, you need to import the component and include it in the `fields` object within `CustomForm.tsx`. The `NumericRangeField` is then passed as a prop to the form component where it can be used like any other field.
```tsx
import NumericRangeField from './path-to-NumericRangeField';
// In CustomForm.tsx
const fields = {
...otherFields,
numericRange: NumericRangeField,
};
// Usage in a form component
<Form fields={fields} ... /> JSON Schema ExampleBelow is an example JSON schema that includes the numeric range field: {
"title": "Example Schema",
"type": "object",
"properties": {
"numericRange": {
"type": "object",
"title": "Numeric Range",
"properties": {
"min": {
"type": "number",
"title": "Minimum Value"
},
"max": {
"type": "number",
"title": "Maximum Value"
}
},
"required": ["min", "max"]
}
}
} This schema defines a numeric range object with UI Options and ValidationThe StylingStyling for the For more detailed styling options, refer to the CSS files mentioned above.
|
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.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- spiffworkflow-frontend/src/components/CustomForm.tsx (5 hunks)
- spiffworkflow-frontend/src/rjsf/custom_widgets/NumericRangeField/NumericRangeField.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- spiffworkflow-frontend/src/components/CustomForm.tsx
- spiffworkflow-frontend/src/rjsf/custom_widgets/NumericRangeField/NumericRangeField.tsx
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.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (3)
- docs/Building_Diagrams/Forms.md (1 hunks)
- spiffworkflow-frontend/src/components/CustomForm.tsx (5 hunks)
- spiffworkflow-frontend/src/rjsf/custom_widgets/NumericRangeField/NumericRangeField.tsx (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- spiffworkflow-frontend/src/components/CustomForm.tsx
Additional comments: 6
docs/Building_Diagrams/Forms.md (4)
234-277: The documentation for the
NumericRangeField
component is clear and provides a good overview, JSON schema example, UI schema example, and validation information. This should help users understand how to implement and use the new feature in their forms.240-260: The JSON schema example for the
NumericRangeField
is correctly structured, with appropriate types and titles for the minimum and maximum values.267-272: The UI schema example correctly associates the
numericRange
property with a custom UI field, indicating that there is a custom component designed to handle the numeric range input.275-277: The validation rule that ensures the maximum value cannot be less than the minimum value is crucial and is correctly documented, which is essential for the correct functioning of the numeric range field.
spiffworkflow-frontend/src/rjsf/custom_widgets/NumericRangeField/NumericRangeField.tsx (2)
60-64: The
formatNumberString
function correctly formats the number with commas. This is a good use of regex for readability.30-155: The
NumericRangeField
component is structured well and follows React best practices for component creation and export.
const parseNumberString = (numberString: string) => | ||
Number(numberString.replace(/,/g, '')); |
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.
The parseNumberString
function has been updated to remove commas, but it still lacks error handling for non-numeric input, which could lead to NaN issues if the input is not a valid number. Consider adding validation to ensure the input is a valid number before attempting to parse it.
onChangeLocal('min', values); | ||
}} | ||
invalid={commonAttributes.invalid} | ||
defaultValue="0" |
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.
The TextInput
components are using both defaultValue
and value
props, which can lead to conflicts as defaultValue
is for uncontrolled components and value
is for controlled ones. Ensure that the components are being used as intended and consider removing defaultValue
if the components are controlled.
Also applies to: 139-139
const onChangeLocal = (nameToChange: any, event: any) => { | ||
event.preventDefault(); | ||
const numberValue = parseNumberString(event.target.value); | ||
if (numberValue > maxNumber) { | ||
return; | ||
} | ||
if (!disabled && !readonly) { | ||
if (nameToChange === 'min' && numberValue > max) { | ||
onChange({ | ||
...(formData || {}), | ||
min: numberValue, | ||
max: numberValue, | ||
}); | ||
} else { | ||
onChange({ | ||
...(formData || {}), | ||
...{ max, min }, | ||
[nameToChange]: numberValue, | ||
}); | ||
} | ||
} |
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.
The onChangeLocal
function uses the any
type for its parameters, which is not type-safe. Consider specifying more accurate types for nameToChange
(which should be a string) and event
(which should be a React.ChangeEvent<HTMLInputElement>
or similar).
spiffworkflow-frontend/src/rjsf/custom_widgets/NumericRangeField/NumericRangeField.tsx
Show resolved
Hide resolved
@Kayvon-Martinez we made some updates. we made the auto-set min and max based on the other value (and switching min and max values) a little less magical. hopefully the result feels ok to you when you use it. because of that, we had to add validation logic, so that is now in place. and a few other things. please reach out if anything seems amiss or if you'd like to talk anything through; we are more than happy to do so. Please take a look at the coderabbit comments and update code if it seems worth updating and then click "Resolve conversation" when you are happy with those. we learned about |
8e91ef5
to
8a14f4a
Compare
I just tested it and I love the changes you made! Thank you! I have confirmed the merge. Thanks again! |
originally from: #799
Implements #647.
Added a custom field to handle numeric ranges. The field checks for the first key in the properties of the schema and will add min and max to it when the values are put in.
Summary by CodeRabbit
New Features
NumericRangeField
component for inputting numeric ranges in forms.Style
rjsf
components, including font size, width, and color adjustments.Documentation
NumericRangeField
feature, including usage examples and validation details.Bug Fixes