ERA-7998: List activity for an incident #813

luixlive · 2022-12-13T22:14:44Z

Ticket: https://allenai.atlassian.net/browse/ERA-7998
Env: https://era-7998.pamdas.org/
Figma: https://www.figma.com/file/1u3VbK9kbOEuUg9Wi8AHRW/Patrol-%26-Report-UI-Refresh-FINAL?node-id=1104%3A22978&t=HzjJn1tgySJt8Qbu-0

Evidence
Desktop:

Mobile:

JoshuaVulcan

Overall, looks amazing and will be very very useful!

I left a few questions/comments/bits of feedback to address.

JoshuaVulcan · 2022-12-14T23:34:55Z

src/ReportManager/ActivitySection/ReportListItem/index.js

@@ -0,0 +1,167 @@
+import React, { memo, useCallback, useEffect, useMemo } from 'react';


There's already a ReportListItem component, do we feel like this is going to be confusing?
And/or is this different enough, functionality-wise, to justify its own component as opposed to just reconciling it into the main ReportListItem instead? Then we could explore fun things like injecting the report preview accordion in other places as well...

Hmmm I would say this is different enough, specially because of the accordion, but Idk, if you think that may be reused I can merge those two components. I fear we will end up with a pretty big ugly file and don't even take advantage of having merged them.

I'd personally recommend having the accordion panel content as a separate component, affiliated with the ReportListItem, and reconcile the two together.

Sounds nice, I'm creating a new global ReportFormSummary that receives report and schema as props, so we can reuse this new way of reading the form values 👍 So now this file is only the "reconciliator" that puts the existing ReportListItem within an accordion header and the new ReportFormSummary in its panel content

JoshuaVulcan · 2022-12-14T23:35:14Z

src/ReportManager/ActivitySection/ReportListItem/styles.module.scss

+
+    .nonSchemaField {
+      width: 50%;
+      display: flex;


nit: alphabetical order is neat for CSS properties

Fixing this PR and doing that from now on. I'm really into alphabetical ordering stuff haha

JoshuaVulcan · 2022-12-14T23:40:34Z

src/ReportManager/ActivitySection/index.test.js

@@ -13,38 +18,55 @@ jest.mock('../../utils/file', () => ({
  fetchImageAsBase64FromUrl: jest.fn(),
 }));

+const server = setupServer(
+  rest.get(


How are you feeling about our use of msw?

100 times better than mocking redux actions. However, Redux is still a pain because in our tests we don't mock our entire redux store, so even though msw mocks the response, we don't have the full store funcitonality of storing the backend data in redux and providing it to the component. This is a limitation to do a full test of certain flows. Anyway, is not a big issue, we can still divide that in two tests: one with the mocked store without data and one with the data preloaded in the test.

Yeah, I'm thinking we'll want to make a slow transition away from mockStore and to just use our real store instead. We honestly don't even use mockStore much for its purpose as intended (checking which actions were dispatched etc), just as a bad placeholder for a store.

Once we're using a real store, the full and actual in situ behavior of a live component can be tested in our test suites, which is awesome.

JoshuaVulcan · 2022-12-14T23:41:45Z

src/ReportManager/ActivitySection/index.test.js

+
+    expect(reportCollapse).toHaveClass('collapse');
+
+    const expandButton = (await screen.findAllByText('arrow-down-simple.svg'))[2];


A couple things:

This should just be a single arrow, rotated with CSS.

You'll need to find a different selector for that element once you've done Develop #1. This is a pretty meh selector anyway

I think you referenced the wrong PR there. Hmm how would you select the icon? I could add a data-testid. That would make it unique, so I won't have an array, but still feels meh to me 😂

lol that was just me typing # 1 and GitHub trying to be smart about it

I think something more closely following the ARIA guidelines for accordions would yield a number of healthy test selectors.

https://www.w3.org/WAI/ARIA/apg/example-index/accordion/accordion.html

JoshuaVulcan · 2022-12-14T23:42:39Z

src/ReportManager/ActivitySection/index.test.js

@@ -133,7 +181,7 @@ describe('ReportManager - ActivitySection', () => {

    expect(noteCollapse).toHaveClass('collapse');

-    const expandButton = (await screen.findAllByText('arrow-down-simple.svg'))[3];
+    const expandButton = (await screen.findAllByText('arrow-down-simple.svg'))[4];


These indexed selectors (findAllByWhatever('hi')[3], findAllByWhatever('hi')[4] etc) have an extremely weak contract as being accurate representations of their elements. We need a better selection pattern.

JoshuaVulcan · 2022-12-14T23:43:58Z

src/ReportManager/ActivitySection/styles.module.scss

@@ -127,6 +127,22 @@
    height: 1.375rem;
    width: 1.375rem;
  }
+
+  &.priority-300 {


Any way to steal this from the other places it's defined in the project?

Hmm I could take a look on how to reuse that CSS. Not an expert with SCSS honestly but I see we already have some mixins for buttons and stuff.

JoshuaVulcan · 2022-12-14T23:45:54Z

src/ReportManager/ActivitySection/ReportListItem/index.js

+            schema={reportSchemas?.schema}
+            showErrorList={false}
+            templates={{
+              ArrayFieldItemTemplate,


So much of this form definition is recycled from elsewhere. Any way to componentize it for regular reuse?

Do you think it's worth it? We can define a CustomForm that is simply the Form implementing the custom components and spreading the ...rest props. It won't do a lot, it's another file to mantain, but we will hide this repetitiveness you mention.

That, or capturing the form defaults in a constant which are spread into this form where it's invoked. Or a combination of the two: define the defaults, give them to a component, use that component as the default form component with overrides available.

JoshuaVulcan · 2022-12-14T23:48:08Z

src/ReportManager/ActivitySection/ReportListItem/index.js

+            </div>
+          </div>
+
+          {reportSchemas?.schema && <Form


What would the level of effort be on concealing empty fields? It seems like we'd need to pass custom input components which are self-concealing when empty, which is kind of a PITA but maybe worth it to avoid things like this:

It's tricky. Hiding it through CSS, if possible, would require a lot of rules. Doing it through the custom components, we would need some new flag that tells the custom component if it is a non editable form print like this, so we can decide not to render the field vs the normal form where eve if it is empty we want the field to show up so the user can edit its value.

Hiding it through CSS, if possible, would require a lot of rules

Funny, that seems like the lazy/cheap way to me. Add an "empty" class to field templates with no values, then select and style those specifically in the context of this panel

I tried this but it's not that simple 😢 I can add the class and try to hide it, display none, etc... But since react-jsonschema-form injects several divs with different grid classes and other stuff, it's really hard to rearrange the form ignoring the empty fields. There's actually an on-going conversation on their repo about this. It is possible to ui:widget: hidden but custom components don't override their uiSchema: rjsf-team/react-jsonschema-form#1236

…omponents, for re-use throughout the new report UI .🤷

…e to refactor them appropriately. minor tweaks to de-anonymize the return values of the useListSortWithButton hook, thus preventing excess renders.

Alcoto95

LGTM 🎉

JoshuaVulcan · 2022-12-15T19:05:11Z

@luixlive just want to call out this ticket comment as something we need to get solid before proceeding

JoshuaVulcan · 2022-12-15T19:24:14Z

Looks like a little chop at the end of the collapse animation

…ure.

Alcoto95 · 2022-12-16T18:17:43Z

@luixlive
Just a comment, will revisit this. I see that the issue for editing just Longitude in events location is happening again:

It is sending only the longitude in the location object. I guess this is more of a question as I don't see the issue in develop (2-64-1 right now), nor Stage (2-63-1 right now). In case you can have a look.

If it can't be solved in this ticket please let me know so that I can reopen the bug 🙌

JoshuaVulcan · 2022-12-16T18:19:10Z

@luixlive Just a comment, will revisit this. I see that the issue for editing just Longitude in events location is happening again:

It is sending only the longitude in the location object. I guess this is more of a question as I don't see the issue in develop (2-64-1 right now), nor Stage (2-63-1 right now). In case you can have a look.

If it can't be solved in this ticket please let me know so that I can reopen the bug 🙌

@Alcoto95 Don't worry about this for now, the hotfix for the location prop currently going into 2.62 (which will then go down into develop and thus up into this branch) includes fixing this.

luixlive · 2022-12-16T18:24:07Z

Ready for new review. But:
1- The pipeline failed. I see you sent a commit fixing the pipeline in another PR, could it be the same thing here @JoshuaVulcan ?
2- @Alcoto95 comment hasn't been addressed. Should we fix that here? Seems out of the scope and I doubt it was injected here.

Alcoto95 · 2022-12-16T18:26:01Z

Alright! Thanks!

JoshuaVulcan

Some more feedback about test quality+structure

JoshuaVulcan · 2022-12-16T23:01:33Z

src/ReportManager/ActivitySection/index.test.js

+    userEvent.click(expandButton);
+
+    await waitFor(() => {
+      expect(reportCollapse).toHaveClass('show');


Testing this css class has no intrinsic link to testing visibility, you just have to take it on faith that a "show" class means something is visible.

We need to assert something something like expect(whatever).toBeVisible()

I don't know how toBeVisible does the validation, but it's not working for this test. Following your suggestions, I should change:
expect(reportCollapse).toHaveClass('collapse'); for expect(reportCollapse).not.toBeVisible();
and
expect(reportCollapse).toHaveClass('show'); for expect(reportCollapse).toBeVisible();
But that doesn't work, even tho I know the test is doing the right thing and the feature works properly 🤔

For reference (even though we already Slacked about it 🙂 )
Need to add the bootstrap styles to your jsdom rendering by either importing the stylesheet in your test file or adding a jest css plugin

Thank you for your answer. Actually, there is an open issue in CRA about this exact scenario (test React Bootstrap Accordion with toBeVisible): facebook/create-react-app#10483 😞

JoshuaVulcan · 2022-12-16T23:01:45Z

src/ReportManager/ActivitySection/index.test.js

+    const reportCollapse = await screen.findByTestId('reportManager-activitySection-collapse-d45cb504-4612-41fe-9ea5-f1b423ac3ba4');
+
+    await waitFor(() => {
+      expect(reportCollapse).toHaveClass('collapse');


ditto as above

JoshuaVulcan · 2022-12-16T23:04:22Z

src/ReportManager/ActivitySection/index.test.js

@@ -181,15 +229,17 @@ describe('ReportManager - ActivitySection', () => {
    const itemsText = (await screen.findAllByRole('listitem')).map((item) => item.textContent.split(' ')[0]);

    expect(itemsText).toEqual([
+      'note.svgNew',


These matchers are not great. Each list item has the text of their icon's name, the name of the file etc, their "new" indicator, etc, all squished together in a weird string. They're also unnecessarily long for showing a basic sort. Can we clean this up to be more legible? Take some known quantities, select for them more cleanly, and assert their position in the list.

JoshuaVulcan · 2022-12-19T18:49:58Z

src/hooks/useReport/index.test.js

@@ -25,6 +25,7 @@ describe('useReport', () => {
      </Provider>
    );

-    expect((await screen.findByTestId('report-data'))).toHaveTextContent('{"displayTitle":"Light","eventTypeTitle":"Light"}');
+    expect((await screen.findByTestId('report-data')))
+      .toHaveTextContent('{"coordinates":[-104.19557197413907,20.75709101172957],"displayPriority":300,"displayTitle":"Light","eventTypeTitle":"Light"}');


This test should really be rewritten using renderHook

[ERA-8002] Time Sort Control

JoshuaVulcan

Looking good. nice work

JoshuaVulcan · 2022-12-21T17:13:48Z

src/hooks/useReport/index.test.js


-    expect((await screen.findByTestId('report-data'))).toHaveTextContent('{"displayTitle":"Light","eventTypeTitle":"Light"}');
+    expect(result.current).toEqual({
+      coordinates: [-104.19557197413907, 20.75709101172957],


So much better!

luixlive added 3 commits December 12, 2022 09:56

ERA-7998: Add report fields in activity

b83b509

ERA-7998: Add complex form field styles

35e269c

Merge branch 'develop' into ERA-7998

ecfa435

luixlive marked this pull request as draft December 13, 2022 22:14

ERA-7998: Add unit tests

00a92e0

luixlive marked this pull request as ready for review December 14, 2022 22:24

ERA-7998: Select widget style fix

c34e69b

luixlive requested review from JoshuaVulcan, Alcoto95 and gaboDev December 14, 2022 23:09

JoshuaVulcan requested changes Dec 14, 2022

View reviewed changes

JoshuaVulcan added 2 commits December 14, 2022 23:56

hook-driven approach to a decoupled pair of list item + sort button c…

af7fc57

…omponents, for re-use throughout the new report UI .🤷

commenting out wonky ActivitySection tests until Ludwig's had a chanc…

f0900b9

…e to refactor them appropriately. minor tweaks to de-anonymize the return values of the useListSortWithButton hook, thus preventing excess renders.

Alcoto95 approved these changes Dec 15, 2022

View reviewed changes

file reorganization as per PR feedback.

28226ed

Daniel Cortés added 2 commits December 15, 2022 13:07

Test fix for pipeline

92a2f90

Need to export a variable to indicate to use the new plugin

1ea3d86

JoshuaVulcan and others added 3 commits December 15, 2022 11:26

render function for button, in case we want to override it in the fut…

d7e9ce5

…ure.

Merge branch 'develop' into ERA-8002

67355ce

ERA-7998: A loooot of improvements follwoing PR suggestions

7c965f0

luixlive requested a review from JoshuaVulcan December 16, 2022 18:22

luixlive and others added 3 commits December 16, 2022 13:26

Merge branch 'develop' into ERA-7998

2db597f

merging up ERA-7998.

7c567b8

Merge branch 'develop' into ERA-8002

561c51c

JoshuaVulcan added 3 commits December 16, 2022 15:15

one clear test for build

d04a3ea

resolving merge conflicts

4109513

resolving merge conflicts

b34ae59

JoshuaVulcan requested changes Dec 19, 2022

View reviewed changes

JoshuaVulcan and others added 2 commits December 19, 2022 10:53

placeholder test file until post-merge

8402948

ERA-7998: Small test improvements

0cba143

luixlive requested a review from JoshuaVulcan December 19, 2022 20:39

JoshuaVulcan added 2 commits December 19, 2022 14:02

merging up ERA-7998, fixing tests

e7e8f40

Merge pull request #814 from PADAS/ERA-8002

d99e01e

[ERA-8002] Time Sort Control

JoshuaVulcan approved these changes Dec 21, 2022

View reviewed changes

ERA-7998: Turning off ff

f628913

luixlive merged commit 26710da into develop Dec 21, 2022

luixlive deleted the ERA-7998 branch December 21, 2022 17:26

		@@ -0,0 +1,167 @@
		import React, { memo, useCallback, useEffect, useMemo } from 'react';


		expect(reportCollapse).toHaveClass('collapse');

		const expandButton = (await screen.findAllByText('arrow-down-simple.svg'))[2];

ERA-7998: List activity for an incident #813

ERA-7998: List activity for an incident #813

Conversation

luixlive commented Dec 13, 2022 • edited Loading

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

JoshuaVulcan Dec 15, 2022 • edited Loading

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

Alcoto95 left a comment

Choose a reason for hiding this comment

JoshuaVulcan commented Dec 15, 2022

JoshuaVulcan commented Dec 15, 2022

Alcoto95 commented Dec 16, 2022

JoshuaVulcan commented Dec 16, 2022 • edited Loading

luixlive commented Dec 16, 2022

Alcoto95 commented Dec 16, 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

JoshuaVulcan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luixlive commented Dec 13, 2022 •

edited

Loading

JoshuaVulcan Dec 15, 2022 •

edited

Loading

JoshuaVulcan commented Dec 16, 2022 •

edited

Loading