Form Engine UI refactor #168

arodidev · 2024-02-06T05:50:19Z

Requirements

This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
My work includes tests or is validated by existing tests.

Summary

This PR aims to tackle various issues:

Responsiveness of the controls rendered on the form.
Refactoring of previous value out of the control component and made to render independently
Fixing previous value for various fields, missing or non-functional
Refactor controls to pull cross-cutting styles from a single stylesheet
Remove previous value functionality from obsGroup fields.
Refactoring questionInfo tooltip positions for controls
Add functionality to dynamically set control width from the form JSON
Fixing control misalignment within the obsGroups

Screenshots

Screen.Recording.2024-02-06.at.08.44.46.mov

Related Issue

Other

pirupius

Nice work with the clean up and refactors. I have some concerns about the naming and duplication of variable names

src/components/inputs/number/ohri-number.component.tsx

src/components/inputs/text/ohri-text.component.tsx

src/components/inputs/ui-select-extended/ui-select-extended.scss

src/components/inputs/unspecified/ohri-unspecified.scss

src/components/previous-value-review/previous-value-review.component.tsx

src/components/section/ohri-form-section.component.tsx

src/components/section/ohri-form-section.scss

src/ohri-form.component.scss

samuelmale

Nice work @arodidev, just a few comments. Maybe as a general comment: There is no linked ticket to define the scope of work which makes the review process tricky. Plus I'm in agreement with harmonizing the cross-cutting stylings but styles specific to input components shouldn't be migrated to the ohri-form-section.sccs file.

src/api/types.ts

src/components/group/ohri-obs-group.scss

src/components/inputs/content-switcher/ohri-content-switcher.component.tsx

src/components/section/ohri-form-section.component.tsx

src/components/section/ohri-form-section.scss

src/submission-handlers/base-handlers.ts

src/utils/common-expression-helpers.ts

src/components/section/ohri-form-section.component.tsx

eudson

Lets merge this and have the enhancements ticketed for a different PR.

CynthiaKamau · 2024-03-05T19:53:42Z

Thanks @arodidev , kindly fix the conflicts

arodidev · 2024-03-06T05:17:05Z

Thanks @arodidev , kindly fix the conflicts

Thanks, resolved.

The unresolved requested changes are blocking the merge button.

pirupius

Some final touch up comments

pirupius · 2024-03-07T07:43:54Z

src/api/types.ts

+}
+export interface previousValue {
+  field: string;
+  value: string | number | Date | boolean | previousValue[];


Which usecase is previousValue[] for? multichoice? Not really comfortable with it

The multi-select input component receives its values as an array of type previousValue

pirupius · 2024-03-07T07:54:37Z

src/components/inputs/content-switcher/ohri-content-switcher.component.tsx

@@ -20,6 +21,15 @@ export const OHRIContentSwitcher: React.FC<OHRIFormFieldProps> = ({ question, on
    }
  }, [question]);

+  useEffect(() => {
+    if (!isEmpty(previousValue)) {
+      const { value } = previousValue;


There's a little bit of inconsistency on how the previous value is being accessed. In different controls.

Due to the differences in datatypes of the previousValues for specific controls, otherwise I think the format is pretty standard across the board, maybe expound a bit?

This isn't a show stopper. There other controls that access it with previousValue.value and others do is this way based on your PR

pirupius · 2024-03-07T07:56:24Z

src/components/inputs/date/ohri-date.component.tsx

@@ -144,15 +154,13 @@ const OHRIDate: React.FC<OHRIFormFieldProps> = ({ question, onChange, handler })
  ) : (
    !question.isHidden && (
      <>
-        <div className={`${styles.formField} ${styles.row} ${styles.datetime}`}>
+        <div className={`${styles.datetime}`}>


Nit picking but we don't need the extra literal since the other styles have been stripped

pirupius · 2024-03-07T09:23:05Z

src/components/inputs/multi-select/ohri-multi-select.component.tsx

@@ -114,9 +119,9 @@ export const OHRIMultiSelect: React.FC<OHRIFormFieldProps> = ({ question, onChan
            readOnly={question.readonly}
          />
        </div>
-        <div className={styles.formField} style={{ marginTop: '0.125rem' }}>
+        <div style={{ marginTop: '0.125rem' }}>


Is this a style that is used in other controls? Let's extract this to the scss file

pirupius · 2024-03-07T09:24:19Z

src/components/inputs/radio/ohri-radio.scss

-  display: flex;
-  flex-direction: row;
-  align-items: center;
+.errorLegend legend {


Indentation on this file looks off. Please format this to 2 spaces

pirupius · 2024-03-07T09:24:50Z

src/components/inputs/select/ohri-dropdown.scss

-  display: flex;
-  flex-direction: row;
-  align-items: baseline;
+.boldedLabel label {


same applies to this file around indenting

pirupius · 2024-03-07T09:28:47Z

src/components/inputs/ui-select-extended/ui-select-extended.tsx

@@ -69,6 +69,16 @@ const UISelectExtended: React.FC<OHRIFormFieldProps> = ({ question, handler, onC
    question.value = handler?.handleFieldSubmission(question, value, encounterContext);
  };

+  useEffect(() => {
+    if (!isEmpty(previousValue)) {


Is this a control we want to add a previous value to? So much is happening already with the search and might have some points of failure or conflict

I see, although previousValue was already supported by ui-select-extended prior to this PR.

arodidev added 17 commits January 18, 2024 21:39

Proof of concept

5b9d05e

Modified prevValue logic on number control

79e2df4

refactor for date control

323a3aa

refactored controls

714dd5b

removed all styles, added prev value for checkbox

100b9a2

refactor select control

d64cdba

Refactor for remaining controls

24b22f1

removed obs group styles and previous value

db6d122

styles cleanup

a4e0c40

cleanup

0be14d5

Changes requested

a1b51c8

realignment of questionInfo

52cf484

label modifications for minor controls

ea084ee

various updates

7b998f7

removing individual control stylesheets

c9be91f

cleanup

ef84b12

Fix bug on toggle control

f429a2b

arodidev marked this pull request as ready for review February 7, 2024 10:10

arodidev requested review from kajambiya and pirupius February 7, 2024 10:10

pirupius reviewed Feb 10, 2024

View reviewed changes

samuelmale previously requested changes Feb 13, 2024

View reviewed changes

arodidev added 3 commits February 14, 2024 20:44

ui-select-xt cleanup

37164cb

PR reviews and general cleanup

54dd620

reinstating control stylesheets

5997750

samuelmale reviewed Feb 20, 2024

View reviewed changes

src/components/section/ohri-form-section.component.tsx Outdated Show resolved Hide resolved

arodidev added 4 commits February 20, 2024 16:48

PR resolutions

8f5bd14

Merge branch 'main' into refactor-previous-value

aa402ab

Update ohri-number.component.tsx

e8e9a78

Update ohri-form-section.component.tsx

efa303c

arodidev requested a review from samuelmale February 21, 2024 04:26

eudson approved these changes Feb 21, 2024

View reviewed changes

arodidev added 2 commits March 4, 2024 13:42

fix useEffect updating previous value

c32c3d4

clean-up

a5cb7cb

Merge branch 'main' into refactor-previous-value

4a2a80b

CynthiaKamau approved these changes Mar 6, 2024

View reviewed changes

pirupius reviewed Mar 7, 2024

View reviewed changes

arodidev added 4 commits March 7, 2024 14:48

Merge branch 'openmrs:main' into refactor-previous-value

c46e94c

removing t.apply error

18f3195

Fixing tab spaces

8500803

Clean up

2b4415f

arodidev merged commit 8c5cf5b into openmrs:main Mar 7, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Form Engine UI refactor #168

Form Engine UI refactor #168

arodidev commented Feb 6, 2024 •

edited

Loading

pirupius left a comment

samuelmale left a comment

eudson left a comment

CynthiaKamau commented Mar 5, 2024

arodidev commented Mar 6, 2024

pirupius left a comment

pirupius Mar 7, 2024

arodidev Mar 7, 2024

pirupius Mar 7, 2024

arodidev Mar 7, 2024

pirupius Mar 7, 2024

pirupius Mar 7, 2024

pirupius Mar 7, 2024

pirupius Mar 7, 2024

pirupius Mar 7, 2024

pirupius Mar 7, 2024

arodidev Mar 7, 2024

Form Engine UI refactor #168

Form Engine UI refactor #168

Conversation

arodidev commented Feb 6, 2024 • edited Loading

Requirements

Summary

Screenshots

Related Issue

Other

pirupius left a comment

Choose a reason for hiding this comment

samuelmale left a comment

Choose a reason for hiding this comment

eudson left a comment

Choose a reason for hiding this comment

CynthiaKamau commented Mar 5, 2024

arodidev commented Mar 6, 2024

pirupius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arodidev commented Feb 6, 2024 •

edited

Loading