Redesigned data entry page #133

ivan-kishko · 2022-11-02T07:15:49Z

Description of the change

Redesigned data entry page implementation

Redesigned summary panel
Redesigned publish data panel
Redesigned data entry form
Add Help Page

Type: Non-breaking refactor

All pull requests must have at least one of the following labels applied (otherwise the PR will fail):

Label	Description
Type: Bug	non-breaking change that fixes an issue
Type: Feature	non-breaking change that adds functionality
Type: Breaking Change	fix or feature that would cause existing functionality to not work as expected
Type: Non-breaking refactor	change addresses some tech debt item or prepares for a later change, but does not change functionality
Type: Configuration Change	adjusts configuration to achieve some end related to functionality, development, performance, or security
Type: Dependency Upgrade	upgrades a project dependency - these changes are not included in release notes

Related issues

Closes #118

Checklists

Development

This box MUST be checked by the submitter prior to merging:

Double- and triple-checked that there is no Personally Identifiable Information (PII) being mistakenly added in this pull request

These boxes should be checked by the submitter prior to merging:

Tests have been written to cover the code changed/added as part of this pull request

Code review

These boxes should be checked by reviewers prior to merging:

This pull request has a descriptive title and information useful to a reviewer
This pull request has been moved out of a Draft state, has no "Work In Progress" label, and has assigned reviewers
Potential security implications or infrastructural changes have been considered, if relevant

ivan-kishko · 2022-11-02T07:24:27Z

Hello all! Have a couple comments

1 test failed due to DataEntryFormTest expects context fields to be in form but according to new design they are removed from there. I already asked @lilidworkin in slack about what should I do with this, but any comments about this would be great to hear.
Disabled disaggregation tab name inside form should be darkgrey when hovered (but not active). When active it turns blue, and currently i did not change blue to darkgrey on hover. If it has to be changed, please let me know, I guess it is question for you @hobuobi.
Thanks!

Update: question about test is solved

mxosman · 2022-11-02T15:30:58Z

Hello all! Have a couple comments

1 test failed due to DataEntryFormTest expects context fields to be in form but according to new design they are removed from there. I already asked @lilidworkin in slack about what should I do with this, but any comments about this would be great to hear.

Disabled disaggregation tab name inside form should be darkgrey when hovered (but not active). When active it turns blue, and currently i did not change blue to darkgrey on hover. If it has to be changed, please let me know, I guess it is question for you @hobuobi.
Thanks!

Hi Ivan! I haven't done a deep dive into the code yet - but just to answer your first question - that's totally fine. Makes sense since the context is no longer in the data entry form and your solution to remove its assertion from the test is perfect.

lilidworkin · 2022-11-02T21:11:50Z

@ivan-kishko this looks great!! A few tiny things:

I think the behavior you have for hovered/active disabled disaggregation tab names is fine! (i.e. turns dark grey on hover, and blue on active).
I did notice that hovering on a non-disabled disaggregation tab name doesn't do anything -- should it turn blue? I think that was the previous behavior.
If only one metric is disabled, the text at the bottom of the page is slightly wrong grammatically. Instead of

Total Staff is associated with your agency’s operations, but have been disabled. If you believe this is incorrect, go to Metric Configuration to re-enable them.

it should be

Total Staff is associated with your agency’s operations, but has been disabled. If you believe this is incorrect, go to Metric Configuration to re-enable it.

Question for @hobuobi -- I found myself missing the "Back to Reports Overview" link we used to have on the left side. Do we not need that anymore since we have "Save as Draft" instead?

lilidworkin · 2022-11-02T21:13:53Z

Oh, and one other tiny thing: in the help panel, the next next to "Breakdowns" seems to be vertically aligned, but instead can it go onto two lines?

That is, instead of

Can it look like

ivan-kishko · 2022-11-02T21:23:14Z

@ivan-kishko this looks great!! A few tiny things:

I think the behavior you have for hovered/active disabled disaggregation tab names is fine! (i.e. turns dark grey on hover, and blue on active).

I did notice that hovering on a non-disabled disaggregation tab name doesn't do anything -- should it turn blue? I think that was the previous behavior.

If only one metric is disabled, the text at the bottom of the page is slightly wrong grammatically. Instead of

Total Staff is associated with your agency’s operations, but have been disabled. If you believe this is incorrect, go to Metric Configuration to re-enable them.

it should be

Total Staff is associated with your agency’s operations, but has been disabled. If you believe this is incorrect, go to Metric Configuration to re-enable it.

Question for @hobuobi -- I found myself missing the "Back to Reports Overview" link we used to have on the left side. Do we not need that anymore since we have "Save as Draft" instead?

@hello @lilidworkin
1.Got it.
2. If you look at the video that attached to this task hovering over not active and non-disabled disaggregation indeed does not do anything (or maybe mouse was not exactly on item and this is my mistake then). Anyway some hover effect seems reasonable, going to fix it.
3.Sorry did not notice, gonna fix it as well

About that link - now we have "Save as Draft" button which does the same, also you can see that in figma this link is removed (not on demo video though)

Fixed with 428bca7 and 4e363fa

…side help page

mxosman · 2022-11-03T16:32:12Z

publisher/src/components/Reports/DataEntryForm.tsx

@@ -125,6 +124,7 @@ const DataEntryForm: React.FC<{

  const isPublished =
    reportStore.reportOverviews[reportID].status === "PUBLISHED";
+  const navigateState = { settingsMenuOption: menuOptions[2] };


publisher/src/components/Forms/NotReportedIcon.tsx

mxosman · 2022-11-03T16:44:08Z

Hi Ivan! This looks fantastic - thank you for swimming in the wild DataEntryForm sea! It's really cool to see these pages evolve and I love a lot of the decisions you made.

Here are a few notes from my end:

When hovering over a disabled disaggregation tab, the tooltip's opacity appears too faint.

Small note: for some reason it feels like the spacing between paragraphs and the line height in the text in the help modal are a bit vertically overcrowded. I'm not sure if Figma has the exact measurements between these areas if it was written in one text box - but I would recommend adding a bit more line height and spacing between paragraphs:
This PR:

Figma:

When the Need Help modal is open, it feels a bit strange to still be able to interact with the data entry form. My recommendation is to disallow any interaction with the layers below the open modal/background.

terryttsai · 2022-11-03T19:12:55Z

Going off of Mahmoud's comment on how we should disable interaction with the layers below the Need Help modal, we could also make it so that clicking outside the need help modal closes the modal?

publisher/src/components/Forms/NotReportedIcon.tsx

terryttsai · 2022-11-03T19:42:16Z

publisher/src/components/Reports/DataEntryForm.tsx

+          <Button type="border" onClick={() => setShowDataEntryHelpPage(true)}>
+            Need Help?
+          </Button>
+          <Button type="border" onClick={() => navigate("/")}>


This is more a question for @hobuobi but should this button be called "Save as Draft" if we already autosave and this just navigates you to the Reports page?

I can imagine users clicking on this button needlessly thinking they need to in order to save their progress

…me help page styled components, disable interaction with form while help page is shown,

ivan-kishko · 2022-11-03T21:27:24Z

Hi Ivan! This looks fantastic - thank you for swimming in the wild DataEntryForm sea! It's really cool to see these pages evolve and I love a lot of the decisions you made.

Here are a few notes from my end:

When hovering over a disabled disaggregation tab, the tooltip's opacity appears too faint.

Small note: for some reason it feels like the spacing between paragraphs and the line height in the text in the help modal are a bit vertically overcrowded. I'm not sure if Figma has the exact measurements between these areas if it was written in one text box - but I would recommend adding a bit more line height and spacing between paragraphs:
This PR:

Figma:

When the Need Help modal is open, it feels a bit strange to still be able to interact with the data entry form. My recommendation is to disallow any interaction with the layers below the open modal/background.

Hello @mxosman , fixed all you asked to in commit 3c07c0a
Have a few comments though:

This hovering - I can't reproduce that, thing is - tooltip has no css properties that would set opacity or something like that, can you tell a bit more what you made to make tooltip look like that.
In fact figma has like description of what font-sizes and line-heights are used in this text (can not understand why it look different), actually these are from predefined typography theme for project. Anyway you are right, I added some more spacing and changed line-height of subtitle to make it look prettier
I did that, but reading what @terryttsai wrote maybe I will have to change it again, will see.

mxosman · 2022-11-03T22:20:12Z

Hello @mxosman , fixed all you asked to in commit 3c07c0a Have a few comments though:

This hovering - I can't reproduce that, thing is - tooltip has no css properties that would set opacity or something like that, can you tell a bit more what you made to make tooltip look like that.

In fact figma has like description of what font-sizes and line-heights are used in this text (can not understand why it look different), actually these are from predefined typography theme for project. Anyway you are right, I added some more spacing and changed line-height of subtitle to make it look prettier

I did that, but reading what @terryttsai wrote maybe I will have to change it again, will see.

Thank you, Ivan!

I figured out that it was happening on the 3rd disaggregation tab. Do you have access to other agencies? If so, if you navigate to the agency list and select Law Enforcement (for this example), go to the Settings > Metric Configuration > Select Total Arrests metric (since it has 3 disaggregation tabs) and disable the 3rd tab (Gender) - attaching a video of the steps:

Screen.Recording.2022-11-03.at.5.10.52.PM.mov

Let me know if you can recreate. It's on report metrics that have 3 disaggregation tabs and the 3rd one is disabled.

OK - I'm glad I'm not the only one - this has stumped me before too - not sure why despite using the same predefined font properties, it looks slightly different. Thank you for taking time to make those adjustments!
To clarify, I think Terry meant in addition to disabling interaction with the below form when the modal is open, we can also set the behavior of clicking outside of the open modal to trigger the modal to close.

terryttsai · 2022-11-03T22:48:20Z

Besides my suggestions, the code looks good to me, thanks Ivan!

…is disabled but has input values and/or errors

ivan-kishko · 2022-11-04T13:03:23Z

Hello @mxosman , fixed all you asked to in commit 3c07c0a Have a few comments though:

This hovering - I can't reproduce that, thing is - tooltip has no css properties that would set opacity or something like that, can you tell a bit more what you made to make tooltip look like that.

In fact figma has like description of what font-sizes and line-heights are used in this text (can not understand why it look different), actually these are from predefined typography theme for project. Anyway you are right, I added some more spacing and changed line-height of subtitle to make it look prettier

I did that, but reading what @terryttsai wrote maybe I will have to change it again, will see.

Thank you, Ivan!

I figured out that it was happening on the 3rd disaggregation tab. Do you have access to other agencies? If so, if you navigate to the agency list and select Law Enforcement (for this example), go to the Settings > Metric Configuration > Select Total Arrests metric (since it has 3 disaggregation tabs) and disable the 3rd tab (Gender) - attaching a video of the steps:

Screen.Recording.2022-11-03.at.5.10.52.PM.mov
Let me know if you can recreate. It's on report metrics that have 3 disaggregation tabs and the 3rd one is disabled.

OK - I'm glad I'm not the only one - this has stumped me before too - not sure why despite using the same predefined font properties, it looks slightly different. Thank you for taking time to make those adjustments!

To clarify, I think Terry meant in addition to disabling interaction with the below form when the modal is open, we can also set the behavior of clicking outside of the open modal to trigger the modal to close.

Thanks for clarifying on steps to repro the issue with hovering disaggregation @mxosman!
So from what I get - it has nothing to do with order but with fill state of dimensions inside disaggregation. This effect you can see when it is disabled but has some filled inputs with or without errors. Basically in this case icon and tooltip have been given additional css with filter property. I just change it to use opacity instead filter and now on hover everything is ok and not hover state looks the same from user perspective. Although not sure why disabled disaggregation keeps values inside its inputs, but I guess this is a question for another day or task)
Made close help page on click away as well. Check out 84a2438 and b00d836 commits to look at result! Thanks!

mxosman · 2022-11-04T14:05:09Z

Thanks for clarifying on steps to repro the issue with hovering disaggregation @mxosman! So from what I get - it has nothing to do with order but with fill state of dimensions inside disaggregation. This effect you can see when it is disabled but has some filled inputs with or without errors. Basically in this case icon and tooltip have been given additional css with filter property. I just change it to use opacity instead filter and now on hover everything is ok and not hover state looks the same from user perspective. Although not sure why disabled disaggregation keeps values inside its inputs, but I guess this is a question for another day or task) Made close help page on click away as well. Check out 84a2438 and b00d836 commits to look at result! Thanks!

Thank you so much for all the changes, Ivan! Very strange about the tooltip! For some reason I'm still seeing it when I run this branch with your latest commits (HEAD = b00d836):

This is on Law Enforcement > Total Arrests -- do you see the same or no?

Everything else looks good to me!

mxosman · 2022-11-04T14:18:01Z

Thanks for clarifying on steps to repro the issue with hovering disaggregation @mxosman! So from what I get - it has nothing to do with order but with fill state of dimensions inside disaggregation. This effect you can see when it is disabled but has some filled inputs with or without errors. Basically in this case icon and tooltip have been given additional css with filter property. I just change it to use opacity instead filter and now on hover everything is ok and not hover state looks the same from user perspective. Although not sure why disabled disaggregation keeps values inside its inputs, but I guess this is a question for another day or task) Made close help page on click away as well. Check out 84a2438 and b00d836 commits to look at result! Thanks!

Thank you so much for all the changes, Ivan! Very strange about the tooltip! For some reason I'm still seeing it when I run this branch with your latest commits (HEAD = b00d836):

This is on Law Enforcement > Total Arrests -- do you see the same or no?

Everything else looks good to me!

It shows up fine when the disaggregation tab is clicked (open) - but when it is not open the tooltip has this grey'd like the screenshot above.

mxosman · 2022-11-04T14:21:27Z

I think I found the culprit - try removing the opacity all together in the DisaggregationHasInputIndicator styled component.

ivan-kishko · 2022-11-04T14:28:46Z

I think I found the culprit - try removing the opacity all together in the DisaggregationHasInputIndicator styled component.

It is going to work but then disabled disaggregations which have inputs filled will not look different from ones that has only empty inputs. Can't understand why It does not work for you.

ivan-kishko · 2022-11-04T14:36:39Z

I think I found the culprit - try removing the opacity all together in the DisaggregationHasInputIndicator styled component.

It is going to work but then disabled disaggregations which have inputs filled will not look different from ones that has only empty inputs. Can't understand why It does not work for you.

@mxosman I figured out why, need some time to resolve it.

ivan-kishko · 2022-11-04T15:23:04Z

@mxosman sorry for that issue was not actually solved. Can you please check ad1697e commit if you have time?

mxosman

@mxosman sorry for that issue was not actually solved. Can you please check ad1697e commit if you have time?

No apologies necessary at all - I'm sure a lot of this is due my past-self's decisions. Thank you so much for taking the time on this - I really like your solution! Much appreciated.

lilidworkin

Thank you, @ivan-kishko ! This should be good to merge! I hope we'll have a new task for you ASAP!

ivankishko added 2 commits November 2, 2022 09:51

redesigned data entry page implementation

e648298

fix prop names

4c7125b

ivan-kishko added the Type: Non-breaking refactor label Nov 2, 2022

ivan-kishko requested review from terryttsai, hobuobi, mxosman and lilidworkin November 2, 2022 07:15

ivan-kishko self-assigned this Nov 2, 2022

ivankishko added 3 commits November 2, 2022 10:30

fix z index of top bar (if overscroll border disappear)

7f4ac0c

remove context assertion from data entry form test

b384f37

fix close help button container width

082f798

fixes: hover disaggregation, disabled metrics text, text alignment in…

428bca7

…side help page

mxosman reviewed Nov 3, 2022

View reviewed changes

publisher/src/components/Forms/NotReportedIcon.tsx Show resolved Hide resolved

terryttsai reviewed Nov 3, 2022

View reviewed changes

fixes: added some spacing and line height inside need help page, rena…

3c07c0a

…me help page styled components, disable interaction with form while help page is shown,

fixes: list text inside help page

4e363fa

ivan-kishko requested a review from mxosman November 3, 2022 22:00

ivankishko added 2 commits November 4, 2022 14:43

fixes: close help page on clickaway

84a2438

fixes: tooltip and not reported icon opacity on disaggregation which …

b00d836

…is disabled but has input values and/or errors

fixes: DisaggregationHasInputIndicator opacity on hover

92fa6bb

fixes: fix success color icon on DisaggregationHasInputIndicator

ad1697e

mxosman approved these changes Nov 4, 2022

View reviewed changes

lilidworkin approved these changes Nov 4, 2022

View reviewed changes

ivan-kishko merged commit 3d71fff into main Nov 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redesigned data entry page #133

Redesigned data entry page #133

ivan-kishko commented Nov 2, 2022 •

edited

Loading

ivan-kishko commented Nov 2, 2022 •

edited

Loading

mxosman commented Nov 2, 2022 •

edited

Loading

lilidworkin commented Nov 2, 2022

lilidworkin commented Nov 2, 2022

ivan-kishko commented Nov 2, 2022 •

edited

Loading

mxosman Nov 3, 2022

mxosman commented Nov 3, 2022

terryttsai commented Nov 3, 2022

terryttsai Nov 3, 2022

terryttsai Nov 3, 2022

ivan-kishko commented Nov 3, 2022

mxosman commented Nov 3, 2022 •

edited

Loading

terryttsai commented Nov 3, 2022

ivan-kishko commented Nov 4, 2022 •

edited

Loading

mxosman commented Nov 4, 2022

mxosman commented Nov 4, 2022

mxosman commented Nov 4, 2022

ivan-kishko commented Nov 4, 2022

ivan-kishko commented Nov 4, 2022

ivan-kishko commented Nov 4, 2022 •

edited

Loading

mxosman left a comment

lilidworkin left a comment

Redesigned data entry page #133

Redesigned data entry page #133

Conversation

ivan-kishko commented Nov 2, 2022 • edited Loading

Description of the change

Type: Non-breaking refactor

Related issues

Checklists

Development

Code review

ivan-kishko commented Nov 2, 2022 • edited Loading

mxosman commented Nov 2, 2022 • edited Loading

lilidworkin commented Nov 2, 2022

lilidworkin commented Nov 2, 2022

ivan-kishko commented Nov 2, 2022 • edited Loading

mxosman Nov 3, 2022

Choose a reason for hiding this comment

mxosman commented Nov 3, 2022

terryttsai commented Nov 3, 2022

terryttsai Nov 3, 2022

Choose a reason for hiding this comment

terryttsai Nov 3, 2022

Choose a reason for hiding this comment

ivan-kishko commented Nov 3, 2022

mxosman commented Nov 3, 2022 • edited Loading

terryttsai commented Nov 3, 2022

ivan-kishko commented Nov 4, 2022 • edited Loading

mxosman commented Nov 4, 2022

mxosman commented Nov 4, 2022

mxosman commented Nov 4, 2022

ivan-kishko commented Nov 4, 2022

ivan-kishko commented Nov 4, 2022

ivan-kishko commented Nov 4, 2022 • edited Loading

mxosman left a comment

Choose a reason for hiding this comment

lilidworkin left a comment

Choose a reason for hiding this comment

ivan-kishko commented Nov 2, 2022 •

edited

Loading

ivan-kishko commented Nov 2, 2022 •

edited

Loading

mxosman commented Nov 2, 2022 •

edited

Loading

ivan-kishko commented Nov 2, 2022 •

edited

Loading

mxosman commented Nov 3, 2022 •

edited

Loading

ivan-kishko commented Nov 4, 2022 •

edited

Loading

ivan-kishko commented Nov 4, 2022 •

edited

Loading