Typescriptize reports files #5079

magicznyleszek · 2024-08-26T06:53:52Z

Checklist

If you've added code that should be tested, add tests
If you've changed APIs, update (or create!) the documentation
Ensure the tests pass
Make sure that your code lints and that you've followed our coding style
Write a title and, if necessary, a description of your work suitable for publishing in our release notes
Mention any related issues in this repository (as #ISSUE) and in other repositories (as kobotoolbox/other#ISSUE)
Open an issue in the docs if there are UI/UX changes

Description

Migrated all reports related code to TypeScript, so it's easier to improve it in future. Also changed the UI responsible for creating new reports, so it's clearer how to add new custom report.

Notes

I didn't want to go too far with refactoring the code, just the bare minimum to make the code work consistently and be mostly readable.

Changes here:

Migrated jsapp/js/components/common/modal.tsx to TypeScript
- Also fixed the Esc key handling of Modal component (this is the old deprecated one). The unmount process wasn't really removing the listener.
Changed the topmost part of "Project > Data > Reports", so now it has a heading similar to other routes, and the reports selection/creation uses more friendly UX (now dropdown is just for selecting a report, and creation of new report is moved out of dropdown to a button next to the dropdown)
Moved all non-TS code from /jsapp/js/components/reports/ to TS.
Cleaned up reports related types from AssetResponse, and defined all the shared types of reports in reportsConstants.ts file
All partial components (the ones being used in reports modals) are now having pending state while things are being saved on Back End. Previously the main reports file was simply reacting to Reflux actions being started, not completed, and it was a weird thing. Part of this is still kinda there, as it would require a bigger refactor.
Added few TODO comments for things that I saw being bad, but didn't want to touch
Created reports.utils.ts file for keeping some reusable functions
Created new ReportsModalTabs component to remove duplicated code being used in two places
Fixed getReportData (from dataInterface.ts) function types, as paginated response doesn't follow our usual PaginatedResponse<T>
Moved txtid function from CoffeeScript file to utils.ts, so it was available for refactored reports code

Related issues

Build atop #5046

…lect

…eports-files # Conflicts: # jsapp/js/popoverMenu.scss

duvld

Besides some questions and a tiny bug the functionality seems all intact. I have a suggestion though, do you think we can split a PR like this into multiple? A typescriptization IMO should be singular and (almost) trivial--if the compiler doesn't complain and we do a quick review then it should be merged. This would remove a lot of noise and confusion if there's a refactor that's coming as well.

jsapp/js/components/reports/reportsConstants.ts

duvld · 2024-09-17T00:04:03Z

jsapp/js/components/reports/reportsConstants.ts

+ * the Chart.js), the latter is the instance of a report style (as Back end
+ * understands it).
+ */
+export const REPORT_STYLES: ReportStyleDefinitions = Object.freeze({


I'm trying to understand this as I don't encounter using mapped types often and at first glace this feels very circular.

So ReportStyleName is just a union of strings that we get from Chart.js. ReportStyleDefinition is an interface that ties up the report style strings from Chart.js with a string label and a chartJsType. In essence the existence of ReportStyleDefinition is to combine together the type, label and Chart.js stuff for each graph type.

Then ReportStyleDefinitions is a mapped type that creates an instance which has each unioned string in ReportStyleName as a value and its type is ReportStyleDefinition. We then manually set REPORT_STYLES to fit this type?

Then we have a ReportStyle type, which by the looks of it are settings/configs that can be applied to each report and change the way it's presented. Do you think this could have a better name, such as ReportStyleSettings?

Yeah, this weird construct is needed so everything ties together in the places that use the const. I renamed REPORT_STYLES to CHART_STYLES (and all the other related things renamed to use "chart" word) to make it less confusing.

duvld · 2024-09-17T00:30:49Z

jsapp/js/components/reports/reportContents.component.tsx

+
+    const reportData = this.props.reportData;
+
+    // TODO: the code below is very convoluted and hard to read/understand, even


Oh my 🥲 if you have some time I would love to know how this works--assuming you had time to figure it out while doing this typescriptization

duvld · 2024-09-17T22:36:49Z

jsapp/js/components/reports/chartColorsPicker.component.tsx

+  defaultStyle: ReportStyle;
+}
+
+export default function ChartColorsPicker(props: ChartColorsPickerProps) {


Why do we switch this one to a functional component? Some of the other typescriptization files remained as class components

duvld · 2024-09-18T00:51:02Z

jsapp/js/components/reports/questionGraphSettings.component.tsx

+import type {FailResponse} from 'js/dataInterface';
+
+interface QuestionGraphSettingsProps {
+  parentState: ReportsState;


Would it be hard to refactor this into a store of some kind? Maybe a good second issue for the furture

duvld · 2024-09-19T16:04:54Z

jsapp/js/components/reports/reportStyleSettings.component.tsx

+      }
+    }
+
+    const tabs: ReportsModalTabName[] = ['chart-type', 'colors'];


Think we talked about this before but just quickly, do we think using enums here is too redundant?

duvld · 2024-09-19T16:11:27Z

jsapp/js/components/reports/reportViewItem.component.tsx

-  // HACK: We no longer keep built PreparedTable in state, it's simply rebuilt
-  // during render. We keep this to ensure that re-render happens when props
-  // change.
-  static getDerivedStateFromProps() {


Not sure if I'm not reading the diffs correctly, but was this method never used? I don't see it removed anywhere except here

duvld · 2024-09-19T22:37:41Z

jsapp/js/popoverMenu.scss

@@ -169,59 +169,6 @@
  }
 }

-.popover-menu--custom-reports {


I didn't see this change mentioned in the description, were these just unused styles?

duvld · 2024-09-19T23:01:08Z

jsapp/js/components/common/modal.tsx

    if (!this.props.disableEscClose && (evt.keyCode === KEY_CODES.ESC || evt.key === 'Escape')) {
      this.props.onClose.call(evt);
    }
  }

-  backdropClick(evt) {
+  backdropClick(evt: TouchEvent) {


This error happens when clicking the backdrop now:

Uncaught TypeError: this is undefined backdropClick modal.tsx:72

magicznyleszek added 5 commits August 15, 2024 12:19

typescriptize CustomReportForm, migrate reports PopoverMenu to KoboSe…

fafacf4

…lect

typescriptize Reports (WIP)

904524b

move all reports code to typescript (WIP, almost done)

fa84e11

fix old modal component esc handling

5b98abe

final set of fixes to typescript code of reports

f7388ed

magicznyleszek changed the base branch from beta to task915-simplify-button-component August 26, 2024 06:55

magicznyleszek marked this pull request as ready for review August 26, 2024 07:15

magicznyleszek added the Front end label Aug 26, 2024

Merge branch 'task915-simplify-button-component' into typescriptize-r…

5a1204d

…eports-files # Conflicts: # jsapp/js/popoverMenu.scss

magicznyleszek requested a review from duvld August 26, 2024 18:21

magicznyleszek assigned duvld Aug 26, 2024

Base automatically changed from task915-simplify-button-component to beta September 3, 2024 22:15

magicznyleszek and others added 3 commits September 4, 2024 00:32

Merge branch 'beta' into typescriptize-reports-files

0d889cd

Merge branch 'beta' into typescriptize-reports-files

e88a4af

Merge branch 'beta' into typescriptize-reports-files

2999a30

duvld requested changes Sep 20, 2024

View reviewed changes

magicznyleszek added 2 commits September 23, 2024 12:28

remove unused width param

a331c32

fix comment

432a2d6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typescriptize reports files #5079

Typescriptize reports files #5079

magicznyleszek commented Aug 26, 2024 •

edited

Loading

duvld left a comment •

edited

Loading

duvld Sep 17, 2024

magicznyleszek Sep 23, 2024

duvld Sep 17, 2024

duvld Sep 17, 2024

duvld Sep 18, 2024

duvld Sep 19, 2024

duvld Sep 19, 2024

duvld Sep 19, 2024

duvld Sep 19, 2024


		const reportData = this.props.reportData;

		// TODO: the code below is very convoluted and hard to read/understand, even

Typescriptize reports files #5079

Are you sure you want to change the base?

Typescriptize reports files #5079

Conversation

magicznyleszek commented Aug 26, 2024 • edited Loading

Checklist

Description

Notes

Related issues

duvld left a comment • 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

magicznyleszek commented Aug 26, 2024 •

edited

Loading

duvld left a comment •

edited

Loading