ERA-7995: Migrate existing report form body into new report UI #805

luixlive · 2022-11-24T20:31:40Z

Ticket: https://allenai.atlassian.net/browse/ERA-7995
Env: https://era-7995.pamdas.org/
Figma: https://www.figma.com/file/1u3VbK9kbOEuUg9Wi8AHRW/Patrol-%26-Report-UI-Refresh-FINAL?node-id=1039%3A8672

Evidence
Desktop:

Mobile:

Notes
As a good boy scout, I took the time not to simply copy and paste our existing React JSON schema form implementation, but to update it to the latest version (I left the existing form in the modals as it was so we will be able to remove the old version of the library once we remove that). Now, as part of this new implementation, I followed the migration process, got rid of unnecessary old props and updated the styles. However, there are some form fields I don't know how to test that I see have some custom styles. @JoshuaVulcan could you help me explaining how to create a custom schema with all those advanced fields to see what styles do they need?

luixlive · 2022-11-29T22:44:31Z

I added a couple event types with custom field types for testing purposes:
MEP arrest

Wildlife checkboxes

tiffanynwong · 2022-11-30T21:26:14Z

Looking so good!! The checkboxes look great! A few comments that may need to be separated out to other tickets but all related:

Can we make all fields 40px in height? It seems like depending on if it's a dropdown, text field, location, or date field the height of the field changes.
I'm a little worried about the legibility of text especially the field labels. (Sorry this is after seeing ER in ops rooms). Working on a typography guide which we could use to make text through out the app more consistent.
Sorry didn't provide a design for arrays but now that I'm seeing it with all the other improvements it would be good to clean it up to be consistent with other components we have. Here's a figma link https://www.figma.com/file/1u3VbK9kbOEuUg9Wi8AHRW/Patrol-%26-Report-UI-Refresh-FINAL?node-id=2776%3A25500
Some things to note in the design:

changed the "new" button to match the report, attachments, note button so that only the save button is blue. Also aligned left so that it's not floating
added back the arrows and used a trash icon we use elsewhere instead of the minus
made the arrows and delete button part of the array so that it's clear that those buttons are for that array
adjusted the padding between array titles and fields

Totally open to discussing the changes we want to make and when @JoshuaVulcan @luixlive

luixlive · 2022-11-30T21:59:30Z

I think it makes sense to work those things in this same ticket. I'd just like to mention that the form is rendered by an external library we use: https://react-jsonschema-form.readthedocs.io/en/latest/ But what we do is to overwrite their styles to give it the ER touch. However, I've faced a couple things that are really tricky to do because of the way they render the DOM nodes and the classes they use. If I find myself in a scenario like that while doing these changes I'll let you know @tiffanynwong 😬

JoshuaVulcan

Arrays need a little love, checkboxes look AMAZING, lots of surface area touched here and it seems to work quite well. I appreciate the error handling for failed schemas, where the form is rendered sans-body instead of leaving you in loading spinner hell.

I have a few questions about code style and test approach, but it's looking really good. V close to shippable.

JoshuaVulcan · 2022-11-30T22:15:28Z

package.json

@@ -7,6 +7,10 @@
  "dependencies": {
    "@fortawesome/fontawesome-svg-core": "^1.2.26",
    "@fortawesome/free-solid-svg-icons": "^5.12.0",
+    "@rjsf/bootstrap-4": "^5.0.0-beta.13",
+    "@rjsf/core": "^5.0.0-beta.13",


Does this mean we can remove react-jsonschema-form-bs4? I'd love to ditch that old stinky fork

Ahh, after we ditch the modals. Cool.

It's removed now. Doesnt' make sense to keep our users waiting if we can deliver upgraded / better-looking forms now 😄

JoshuaVulcan · 2022-11-30T22:32:25Z

src/ReportManager/ReportDetailView/index.js

@@ -231,6 +233,21 @@ const ReportDetailView = ({
    reportTracker.track('Change Report Location');
  }, [reportForm, reportTracker]);

+  const onJsonFormChange = useCallback((event) => {


Any reason to surface jsonForm instead of just form here?

Answering all those comments here: It's simply me being over-descriptive. I'll remove the json prefix.

JoshuaVulcan · 2022-11-30T22:33:15Z

src/ReportManager/ReportDetailView/index.js

+  const onJsonFormChange = useCallback((event) => {
+    setReportForm({ ...reportForm, event_details: { ...reportForm.event_details, ...event.formData } });
+
+    reportTracker.track('Change Report Json Form Data');


Similar to ^above, why would our analytics care that the form is json? Wouldn't "Change report form data" be more human-friendly and meaningful?

JoshuaVulcan · 2022-11-30T22:33:22Z

src/ReportManager/ReportDetailView/index.js

+    reportTracker.track('Change Report Json Form Data');
+  }, [reportForm, reportTracker]);
+
+  const onJsonFormError = (errors) => {


onFormError....

JoshuaVulcan · 2022-11-30T22:33:33Z

src/ReportManager/DetailsSection/index.js

+  jsonFormUISchema,
+  onJsonFormChange,
+  onJsonFormError,
+  onJsonFormSubmit,


onFormSubmit

JoshuaVulcan · 2022-11-30T22:33:37Z

src/ReportManager/DetailsSection/index.js

 const DetailsSection = ({
+  jsonFormSchema,
+  jsonFormUISchema,
+  onJsonFormChange,


onFormChange

JoshuaVulcan · 2022-11-30T22:33:46Z

src/ReportManager/DetailsSection/index.js

+  jsonFormSchema,
+  jsonFormUISchema,
+  onJsonFormChange,
+  onJsonFormError,


onFormError

JoshuaVulcan · 2022-11-30T22:36:14Z

src/ReportManager/index.test.js


  beforeEach(() => {
    AddReportMock = jest.fn(() => null);
    AddReport.mockImplementation(AddReportMock);
+    fetchEventTypeSchemaMock = jest.fn(() => () => {});


Didn't we agree to start using msw for API interactions, instead of stubbing the fetch functions?

Ohh, it's the habit

JoshuaVulcan · 2022-11-30T22:37:09Z

src/SchemaFields/index.js

+
+  const uiOptions = getUiOptions(uiSchema);
+  const TitleFieldTemplate = getTemplate('TitleFieldTemplate', registry, uiOptions);
+  const DescriptionFieldTemplate = getTemplate('DescriptionFieldTemplate', registry, uiOptions);


Interesting new pattern on rsjf's part

Your comment reminds me that I simply removed the old code and that probably broke the modals forms 😅 I'll have to add the feature flag check in here

No longer needed, I removed the old version of the library. Now even the modals are using these new forms 👍

luixlive · 2022-12-05T19:44:40Z

@JoshuaVulcan @gaboDev @Alcoto95 this is ready for a review 👍

JoshuaVulcan

We need a loading indicator for the new report body component. Look what happens for a small schema when I throttle my connectivity down a bit:

luixlive · 2022-12-06T15:37:04Z

@JoshuaVulcan Totally, I like that idea. However, should we use our current spinner? I don't think it matches the beauty of our new form 😂 Maybe @tiffanynwong could help with a new icon / design for the "form loader"?

Alcoto95

LGTM now

tiffanynwong · 2022-12-07T00:18:38Z

We need a loading indicator for the new report body component. Look what happens for a small schema when I throttle my connectivity down a bit:

We have a this spinner that is displayed when the report list takes time to load. Spoke to Ludwig and agree it could use fixing but we should keep our spinners consistent so if we change it here it should change in the report list as well.

If we're going to tidy up the spinner. Can we use a circular indeterminate spinner like this? https://mui.com/material-ui/react-progress/ And also change the color to bright blue #0056C7. Here's an example https://www.figma.com/file/1u3VbK9kbOEuUg9Wi8AHRW/Patrol-%26-Report-UI-Refresh-FINAL?node-id=2964%3A25128&t=JWUQGpSKERPdwnVm-1

Using a skeleton would be really cool but probably not worth the effort for the impact. https://mui.com/material-ui/react-skeleton/

JoshuaVulcan · 2022-12-07T15:11:47Z

"Arrest with array" event type layout has some button/input overlap funk going on btw

luixlive · 2022-12-07T19:19:46Z

@tiffanynwong actually updating the current loader would take way more time than I expected. I did the change in the form loader:

But doing it in every other spinner in the app would require a lot of effort. Should I keep the new design in the report's form?

luixlive · 2022-12-07T19:20:28Z

"Arrest with array" event type layout has some button/input overlap funk going on btw

Hmm could you tell me exactly where? I see the MEP arrest report with lots of arrays and all of them render nicely.

JoshuaVulcan · 2022-12-07T19:23:45Z

I pulled the "Arrest with array" event type from develop and put it in your feature environment, you can repro there.

JoshuaVulcan · 2022-12-07T19:24:10Z

@tiffanynwong actually updating the current loader would take way more time than I expected. I did the change in the form loader:

But doing it in every other spinner in the app would require a lot of effort. Should I keep the new design in the report's form?

Yes, please do keep the new spinner in the report form, it looks much better

tiffanynwong · 2022-12-07T19:31:56Z

@tiffanynwong actually updating the current loader would take way more time than I expected. I did the change in the form loader:
But doing it in every other spinner in the app would require a lot of effort. Should I keep the new design in the report's form?

Yes, please do keep the new spinner in the report form, it looks much better

Can you check to make sure the blue is the same blue as the save button please :D

luixlive · 2022-12-07T19:41:04Z

I pulled the "Arrest with array" event type from develop and put it in your feature environment, you can repro there.

I fixed the margin at the top, but the button is still a bit out of phase in the horizontal axis. This is because the only way to add a space between the form fields when there is more than one column per row was by adding a padding at the both sides of each column. That's how they do it and that's the only way I could, because of the way their grid system works. The button is not wrapped by a column, so it doesn't get that horizontal padding:

If this is important I'm open to any suggestions to fix it 😅

Alcoto95

LGTM 🎉 😄

JoshuaVulcan · 2022-12-08T22:43:21Z

The spinner seems to show indefinitely for schema requests which error out (such as "Test Hugo" in develop right now), instead of cancelling/showing a meaningful error message. If an error message is outside the scope of this task, maybe at least hiding the spinner once the request is done, regardless of success/failure.
Datetime fields have extra top margins:
Some new console errors and warnings are showing up. Some seem minor, like an HTML structural validity issue and a couple propTypes tweaks. See below:

Warning: validateDOMNesting(...): <button> cannot appear as a descendant of <button>.

Warning: Failed prop type: The prop `formSchema` is marked as required in `DetailsSection`, but its value is `undefined`.

Warning: Failed prop type: The prop `formUISchema` is marked as required in `DetailsSection`, but its value is `undefined`.

uiSchema.classNames' is deprecated and may be removed in a major release; Use 'ui:classNames' instead.

Warning: Received `true` for a non-boolean attribute `custom`.

If you want to write it to the DOM, pass a string instead: custom="true" or custom={value.toString()}.

Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?

Can we tighten up the line-spacing property on the labels? It seems excessively "tall" right now when the label wraps:

JoshuaVulcan

see above comment. This is getting suuuper close to being ready!

luixlive · 2022-12-12T15:51:22Z

@JoshuaVulcan thanks! I didn't notice the warnings. I'll work on these things. I could add the error message if it's trivial. But if we require designs again, maybe I agree that we already loaded this task with a lot of stuff. We would require a third QA pass.

luixlive · 2022-12-12T20:46:38Z

@JoshuaVulcan about the warnings:

1- The first one you mention, a button being inside a button, was not injected in this PR and seems like a rendering issue in the ReportMenu component. I didn't debug properly, but looks like the Toggle receives a prop as="button" and it wraps the KebabMenuIcon which is a button too. We could refactor that, but I don't think it should be part of this task.

2- You didn't mention one about enumNames being deprecated. But I think that will require a complete different ticket, where we also will need to notify the support team since looks like most of our reports use that property which is not standard and will probably be deprecated from react-jsonschema-form in favor of oneOf as is mentioned here: rjsf-team/react-jsonschema-form#532

luixlive added 2 commits November 24, 2022 13:55

ERA-7995: Add react json schema form to new reports UI

ae55275

ERA-7995: Fix existing unit tests

14b17f3

luixlive marked this pull request as draft November 24, 2022 21:30

ERA-7995: Add tests

ef837a7

Base automatically changed from ERA-7994 to develop November 29, 2022 00:34

luixlive added 2 commits November 29, 2022 09:31

Merge branch 'develop' into ERA-7995

1f4d1ff

ERA-7995: Add styles for advanced custom fields

480e0c3

luixlive marked this pull request as ready for review November 29, 2022 22:42

luixlive requested review from JoshuaVulcan, Alcoto95 and gaboDev November 29, 2022 22:42

JoshuaVulcan requested changes Nov 30, 2022

View reviewed changes

luixlive added 4 commits December 2, 2022 17:56

ERA-7995: Update custom components for react json schema form

19cf538

Merge branch 'develop' into ERA-7995

b207c8b

ERA-7995: PR feedback

08be795

ERA-7995: Small styling improvements

8e3b83b

luixlive requested a review from JoshuaVulcan December 5, 2022 19:42

ERA-7995: Hide move buttons in small views

4c3f354

JoshuaVulcan requested changes Dec 6, 2022

View reviewed changes

Alcoto95 approved these changes Dec 6, 2022

View reviewed changes

Merge branch 'develop' into ERA-7995

c47ea63

ERA-7995: Fix margin top in array fields

3999b91

luixlive requested review from JoshuaVulcan and Alcoto95 December 7, 2022 19:41

ERA-7995: Set loader color to bright blue value

edf8e0f

Alcoto95 approved these changes Dec 7, 2022

View reviewed changes

JoshuaVulcan requested changes Dec 8, 2022

View reviewed changes

ERA-7995: Fix console warnings and small styling issues

210963f

luixlive requested a review from JoshuaVulcan December 12, 2022 20:46

JoshuaVulcan approved these changes Dec 12, 2022

View reviewed changes

JoshuaVulcan merged commit b835143 into develop Dec 12, 2022

JoshuaVulcan deleted the ERA-7995 branch December 12, 2022 22:05

ERA-7995: Migrate existing report form body into new report UI #805

ERA-7995: Migrate existing report form body into new report UI #805

Conversation

luixlive commented Nov 24, 2022 • edited Loading

luixlive commented Nov 29, 2022

tiffanynwong commented Nov 30, 2022

luixlive commented Nov 30, 2022

JoshuaVulcan 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

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

luixlive commented Dec 5, 2022

JoshuaVulcan left a comment

Choose a reason for hiding this comment

luixlive commented Dec 6, 2022

Alcoto95 left a comment

Choose a reason for hiding this comment

tiffanynwong commented Dec 7, 2022

JoshuaVulcan commented Dec 7, 2022

luixlive commented Dec 7, 2022

luixlive commented Dec 7, 2022

JoshuaVulcan commented Dec 7, 2022

JoshuaVulcan commented Dec 7, 2022 • edited Loading

tiffanynwong commented Dec 7, 2022

luixlive commented Dec 7, 2022

Alcoto95 left a comment

Choose a reason for hiding this comment

JoshuaVulcan commented Dec 8, 2022 • edited Loading

JoshuaVulcan left a comment • edited Loading

Choose a reason for hiding this comment

luixlive commented Dec 12, 2022

luixlive commented Dec 12, 2022

luixlive commented Nov 24, 2022 •

edited

Loading

JoshuaVulcan commented Dec 7, 2022 •

edited

Loading

JoshuaVulcan commented Dec 8, 2022 •

edited

Loading

JoshuaVulcan left a comment •

edited

Loading