(feat) O3-3230: Add rule builder to interactive builder #450
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +43.9 kB (+1.11%) Total Size: 4.02 MB
ℹ️ View Unchanged
|
|
I swear to god, this e2e test passes locally |
NethmiRodrigo
force-pushed
the
feat/rule-builder
branch
from
e5217ba to
36cb0b8
Compare
ibacher
reviewed
Apr 10, 2025
There was a problem hiding this comment.
Thanks @NethmiRodrigo! I realize a bunch of these are nit-picky, but I think one or two of these really need to be addressed.
| @@ -21,6 +21,7 @@ module.exports = { | |||
| '^dexie$': '<rootDir>/node_modules/dexie', | |||
| '^react-i18next$': '<rootDir>/__mocks__/react-i18next.js', | |||
| 'react-markdown': '<rootDir>/__mocks__/react-markdown.tsx', | |||
| '^uuid$': '<rootDir>/node_modules/uuid', | |||
There was a problem hiding this comment.
I'd probably prefer either 'uuid' or 'require.resolve('uuid')' here rather than hard-coding a path in node_modules.
| @@ -88,6 +88,7 @@ | |||
| "@typescript-eslint/eslint-plugin": "^7.9.0", | |||
| "@typescript-eslint/parser": "^7.9.0", | |||
| "css-loader": "^6.11.0", | |||
| "dayjs": "^1.11.11", | |||
There was a problem hiding this comment.
IIRC, dayjs should be parked as a peer dep.
|
|
||
| export const RuleProvider: React.FC<RuleProviderProps> = ({ children }) => { | ||
| const [rules, setRules] = useState<Array<FormRule>>(); | ||
| const cachedRules = useMemo(() => rules, [rules]); |
There was a problem hiding this comment.
I don't think there's a good reason to use useMemo() on a result from useState(). This seems like pure overhead.
| export const RuleProvider: React.FC<RuleProviderProps> = ({ children }) => { | ||
| const [rules, setRules] = useState<Array<FormRule>>(); | ||
| const cachedRules = useMemo(() => rules, [rules]); | ||
| const updateRules = useCallback((newRules: Array<FormRule>) => setRules(newRules), [setRules]); |
There was a problem hiding this comment.
Ditto here. Instead of wrapping in a callback, just pass setRules from useState()
Comment on lines
+27
to
+31
| const rulesForQuestionIndex = rules?.findIndex((rule: FormRule) => rule?.question === question?.id); | ||
| const rulesForQuestion = | ||
| rulesForQuestionIndex !== -1 && rulesForQuestionIndex !== undefined | ||
| ? rules?.filter((rule: FormRule) => rule?.question === question?.id) | ||
| : [{ id: uuidv4(), isNewRule: false, question: question?.id }]; |
There was a problem hiding this comment.
This seems like it could be re-written to be much simpler:
Suggested change
| const rulesForQuestionIndex = rules?.findIndex((rule: FormRule) => rule?.question === question?.id); | |
| const rulesForQuestion = | |
| rulesForQuestionIndex !== -1 && rulesForQuestionIndex !== undefined | |
| ? rules?.filter((rule: FormRule) => rule?.question === question?.id) | |
| : [{ id: uuidv4(), isNewRule: false, question: question?.id }]; | |
| const rulesForQuestion = rules?.filter((rule: FormRule) => rule?.question === question?.id) ?? | |
| [{ id: uuidv4(), isNewRule: false, question: question?.id }]; |
|
|
||
| interface CustomComboBoxProps { | ||
| id: string; | ||
| items: Array<Record<string, string>>; |
There was a problem hiding this comment.
Shouldn't the type here be a little more specific? I.e., we require a label property for this to work and it seems like we also expect a concept property... and ignore anything else.
| } | ||
| .combobox .multiSelect { | ||
| position: relative; | ||
| bottom: 8px;// Adjust the z-index to a higher value to ensure it's on top |
There was a problem hiding this comment.
- There are a bunch of rules here specified using
px. All of these should be changed toremmeasurements. - The comment here is, um... ???
Comment on lines
+130
to
+132
| const prevPageIndex = useRef(-1); | ||
| const prevSectionIndex = useRef(-1); | ||
| const prevQuestionIndex = useRef(-1); |
There was a problem hiding this comment.
I'm a little concerned that these are really intended to be states rather than refs.
| * Generates the condition expression based on the provided parameters. | ||
| */ | ||
| const getSchemaCondition = ( | ||
| condition: string, |
There was a problem hiding this comment.
Shouldn't condition here have a stricter type, i.e., to correspond to the set of known conditions?
| */ | ||
| const getArguments = useCallback((expression: string) => { | ||
| switch (expression) { | ||
| case 'BMI': |
There was a problem hiding this comment.
In general, we seem to have a lot of these that switch on labels rather than some sort of language-independent key, which means that this makes it harder to (eventually) translate this part of the app.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requirements
Summary
This PR integrates rule builder component into form builder, which provides an user-interface rule builder to build conditional logic for the forms in the form builder. Basically the PR #279, I've made a new PR because I worked off a new branch.
This feature is not under a feature flag, because I'd like us to get feedback on this.
Screenshots
Related Issue
https://issues.openmrs.org/browse/O3-3230
Other