DataTable refactor/rewrite

[josh-griffin]

Description

Old table is bad. We're rewriting it. See context (below) or msupply-foundation/react-native-data-table#46

Plan

We'll operate a local import of code for the data table. At the end we'll take the core parts back to react-native-data-table repo

Branches

Feature branch: #1043-data-table-rewrite

Tasks

Each task to have its own issue, branch and PR.

Utility

• Styles
  • New Props
    • ISSUE: New Data Table components require new props #1119
    • PR: #1119 data table new component props #1120
  • Styles
    • ISSUE: CustomerInvoicePage new DataTable #1090
    • PR: #1099 Data table styles #1121
  • Row onPress
    • ISSUE: New DataTable clickable rows #1145
    • PR: #1145 onPress prop to Row component #1146
  • Optimized virtualized list props
    • ISSUE: New DataTable added virtualized list props #1140
    • PR: #1140 data table default props optimisations #1150
  • Factoring renderCells
    • ISSUE: Factor out renderCells from page components #1159
    • PR: #1159 Factoring render cells #1163
  • Factoring renderModal
    • ISSUE: Factor out renderModalContent from page components #1160
    • PR: #1160 Factor out modals #1168
  • New ExpiryInputCell
    • ISSUE: Update ExpiryTextInput component #1155
    • PR: #1155 New ExpiryDateInput cell #1161
  • Replacing SearchBar component
    • ISSUE: SearchBar shouldn't have internal state #1176
    • PR: #1176 New SearchBar component #1178
  • Handle finalizing records not updating state
    • ISSUE: CustomEffect to detect side-effect changes  #1171
    • PR: #1171 useDatabaseChangeListener hook #1174
  • Handle navigation side effects not updating state
    • ISSUE: Custom hook refreshing top route #1172
    • PR: #1172 useNavigationFocusRefresh custom hook #1175
  • Programmatically scrolling data table.
    • ISSUE: Programmatically scrolling new DataTable #1181
    • PR: #1181 New DataTable auto-scroll and focus logic #1187
  • Tidy up flux methods
    • ISSUE: Tidy up of reducers and actions #1169
    • PR: #1169 Flux cleanup #1204
  • Dynamic actions
    • ISSUE: DataTable dynamic actions #1210
    • PR: #1210 DataTable Dynamic actions -- 1 #1213
  • Column update
    • ISSUE: New DataTable Columns  #1209
    • PR: #1209 DataTable columns update -- 2 #1214
  • small updates
    • ISSUE: Various small updates to DataTable branch #1252
    • PR:
  • Column typing
    • ISSUE: Column types should be constant #1250
    • PR: Column types should be constant #1250
  • State Initialisers
    • ISSUE: State initialisers should be extracted #1249
    • PR:
  • Price and unit columns
    • ISSUE: SupplierRequisition price unit columns not implemented #1248
    • PR: SupplierRequisition price unit columns not implemented #1248
  • useCallback calls
    • ISSUE: A lot of no-dependency useCallback calls #1247
    • PR: A lot of no-dependency useCallback calls #1247
  • useSyncListener
    • ISSUE: useSyncListener uses the wrong database #1246
    • PR: useSyncListener uses the wrong database #1246

Pages

Need to rewrite all pages to use new data table.

• CustomerInvoicePage for reference on all following pages
  • Basic component
    • ISSUE: CustomerInvoicePage new DataTable #1090
    • PR: #1090 customer invoice new data table/refactor start #1108
  • Sorting feature
    • ISSUE: New CustomerInvoicePage sorting feature #1110
    • PR: #1110 New CustomerInvoicePage sorting feature #1114
  • Filtering feature
    • ISSUE: New CustomerInvoicePage filtering feature #1111
    • PR: #1111 New CustomerInvoicePage filtering feature #1115
  • data-type synchronization finish later mSupplyMobileAppContainer causing excessive re-renders #1123 blocker
    • ISSUE: New CustomerInvoicePage data type synchronization feature #1112
    • PR: #1112 New CustomerInvoicePage data types shnychronizationh feature #1116
  • Refactor#1
    • ISSUE: New CustomerInvoicePage factoring #1113
    • PR: #1113 New CustomerInvoicePage refactor #1117
  • Refactor#2
    • ISSUE: New DataTable post refactor porting #1137
    • PR: #1136 New CustomerInvoicePage refactor two #1143
  • Tidy up
    • ISSUE: Final new CustomerInvoicePage changes #1148
    • PR: Final new CustomerInvoicePage changes #1148
  • TidyUp2
    • ISSUE: CustomerInvoicePage to use PageActions #1218
    • PR: #1218 CustomerInvoicePage fixes -- 9 #1224
• CustomerInvoicesPage
  • ISSUE: Refactor CustomerInvoicesPage to use new DataTable #1125
  • PR: #1125 customer invoices page new data table #1164
  • FOLLOW UP ISSUE: CustomerInvoicesPage to use PageActions #1220
  • FOLLOW UP PR: #1220 CustomerInvoicesPage fixes -- 8 #1223
• CustomerRequisitionPage
  • ISSUE: Refactor CustomerRequisitionPage to use new DataTable #1126
  • PR: #1126 CustomerRequisitionPage refactor -- 12 #1227
• CustomerRequisitionsPage
  • ISSUE: Refactor CustomerRequisitionsPage to use new DataTable #1127
  • PR: #1129 CustomerRequisitionsPage refactor -- 11 #1226
• StockPage
  • ISSUE: Refactor StockPage to use new DataTable #1128
  • PR: #1128 stock page refactor #1257
• StocktakeEditPage
  • ISSUE: Refactor StocktakeEditPage to use new DataTable  #1129
  • PR: #1129 New StocktakeEditPage - #5 #1192
  • ISSUE2: Refactor StocktakeEditPage - reasons #1186
  • PR2: #1186 StocktakePage reasons logic - #6 #1193
  • ISSUE3: DataTable StocktakeEditPage fixes #1201
  • PR3: #1201 StocktakeEditPage fixes -- 5 #1217
  • ISSUE4: DataTable StocktakeEditPageWithReasons fixes #1199
  • PR4: #1199 Stocktake reasons fixes -- 6 #1221
• StocktakeManagePage
  • ISSUE: Refactor StocktakeManagePage to use new DataTable #1130
  • PR: #1130 New StocktakeManagePage - #4 #1191
  • ISSUE2: DataTable StocktakeManagePage fixes #1198
  • PR2: #1198 StocktakeManagePage fixes -- 4 #1216
• StocktakesPage
  • ISSUE: Refactor StocktakesPage to use new DataTable #1131
  • PR: #1131 New StocktakesPage - #3 #1190
  • ISSUE2: DataTable StocktakePage fixes #1197
  • PR2: #1198 StocktakeManagePage fixes -- 4 #1216
• SupplierInvoicePage
  • ISSUE: Refactor SupplierInvoicePage to use new DataTable #1132
  • PR: #1132 New SupplierInvoicePage #1177
  • ISSUE2: SupplierInvoicePage to use PageActions #1219
  • PR2: #1219 SupplierInvoicePage fixes -- 7 #1222
• SupplierInvoicesPage
  • ISSUE: Refactor SupplierInvoicesPage to use new DataTable #1133
  • PR: #1133 SupplierInvoicesPage refactor -- 10 #1225
• SupplierRequistionsPage
  • ISSUE: Refactor SupplierRequisitionsPage to use new DataTable #1135
  • PR: #1135 New SupplierRequisitionsPage - #1 #1188
  • ISSUE2: DataTable SupplierRequisitionsPage fixes #1195
  • PR: #1134 New SupplierRequisitionPage - #2 #1189
• SupplierRequisitionPage
  • ISSUE: Refactor SupplierRequisitionPage to use new DataTable #1134
  • PR: #1134 New SupplierRequisitionPage - #2 #1189
  • ISSUE2: DataTable SupplierRequisitionPage fixes #1196
  • PR2: #1196 SupplierRequisitionPage fixes #1208
• Stocktake batch modal
  • ISSUE: Refactor StocktakeBatchModal to use new DataTable #1138
  • PR: #1138 StocktakeBatchModal refactor -- 13 #1229
• Regimen Modal
  • ISSUE: DataTable refactor ViewRegimenDataModal #1228
  • PR: #1228 RegimenDataModal refactor -- 14 #1230

Post refactor

• Move data table code to react-native-data-table and publish
  • ISSUE: Design new API react-native-data-table#47
  • PR:
• Use npm package in mobile rather than local
  • ISSUE: New DataTable post refactor porting #1137
  • PR: https://github.com/sussol/react-native-data-table/pulls
• Remove old unused code
  • ISSUE: Remove unused code after DataTable refactor #1165
  • PR:
• Merge to develop
  • ISSUE: DataTable refactor/rewrite #1043
  • PR: #1043 Data table rewrite #1355
• Testing
  • ISSUE: DataTable Refactor Testing #1231

Additional existing issues

These issues should either be fixed or closed with the new data table implementation

• java.lang.RuntimeException · android.os.TransactionTooLargeException: data parcel size 525792 bytes #992
• java.lang.NullPointerException in DrawableContainer.java:888 #792
• java.lang.NullPointerException in DrawableContainer.java:884 #745
• java.lang.NullPointerException in DrawableContainer.java:888 #742
• java.lang.NullPointerException in DrawableContainer.java:823 #722
• java.lang.NullPointerException in DrawableContainer.java:884 #683
• java.lang.NullPointerException in DrawableContainer.java:888 #674
• java.lang.NullPointerException in DrawableContainer.java:884 #651
• java.lang.NullPointerException in DrawableContainer.java:823 #650
• Bugsnag: TypeError · undefined is not an object (evaluating 'a[t]') #621
• Stocktake batch TextInputs clipped #603
• Keyboard not vanishing when tapping on all parts of screen #437
• Date field hides date when trying to edit existing date #517
• Can't view comment on finalised invoices #379
• Finalise for customerInvoice shows No Quantity warning even when items were added and quantities filled out #236
• Customer invoice should warn if allocating more than stock available #77
• When searching for an item in stock table, have to tap twice to expand item #218
• Strange spacing in modalStrings #1036

Context

Old data table was slow, using deprecated ListView, A specific Realmjs version of the deprecated ListView component. Generic table page has significant problems with multiple unnecessary full re-renders.

Design discussion

Addition context here msupply-foundation/react-native-data-table#46
If necessary, discuss GenericPage/GenericTablePage design. Going to have a crack at CustomerInvoicePage to get a solid grasp of the challenges.

Architecture

[Chris-Petty]

Would be better here IMO https://github.com/sussol/react-native-data-table/issues

Right questions though:

• Are we keeping realm? Or, if we can't decide this, are we writing this new data table in such a way that is completely decoupled from realm? (I think coupling the data table with realm, if we are using it, offers quite a few benefits)

Decoupled. It's annoying for people wanting to use the table with anything other than realm. The coupling is Realm.ListView, but we're throwing that out with the rewrite anyway.

• Are we going to keep the data table as a separate NPM package? I don't like having the table abstracted away, personally, but I can also see the benefit

I think executed well, it's a pretty sensible package to exist in it's own right. I'm not against getting generic table page into mobile, though. 2 levels out is exponentially more of a pain in the ass than 1 level.

• Are we using hooks? (This will require upgrading react-native, which should probably happen regardless, but will have to happen sooner rather than later. Keeping in mind the vaccine module branch is already a version behind)

I say yes, though we're trading off backwards compatibility with older RN apps, with modern react APIs/style. Not that they are planning to deprecate the class API.

• FlatList/VirtualizedList/Something else? - FlatList is reall easy to use, whereas virtualized list offers more flexibility. I think FlatList would probably be sufficient.

I think FlatList would probably be sufficient, but I appreciate the flexibility of not being locked to down to Arrays.

• What 'extra's' are needed? Expansions, sections?, custom components as cells, sorting, "switching" (i.e, click a 'menu' button on a row to offer different options), highlighting rows - I think at least thinking about this is a good idea as it can change a lot about how the table is written.

"Yes"

• Performance metrics I think having baselines for rendering performance might be a good idea to have in place at the start, although this could be premature

Yes

• Tests I think having jest upgraded and able to run tests would be extremely valuable

Yes

• refreshData I think the use of refreshData is basically the biggest flaw in the current implementation and that it should not be used at all :)

Yes

Live examples Written in Expo or RN. Should flex the whole API. Might tie into automated tests.

[Chris-Petty]

To be able to use the data table with 5,000 lines as effeciently and performant as with 1 line.

That's a pretty optimistic bar, but here's hoping there aren't hurdles haha.

[craigdrown]

Refactoring data table for mobile

[Chris-Petty]

Reopening this issue as epic

[josh-griffin]

Just need to port code to react-native-data-table repo, publish a new version, use that new version in mobile and remove the then-unused data table code

[wlthomson]

In terms of testing, this can be moved to "Done" as soon as the remaining issues are tested.

[kat-ms]

Closing - looks more or less done (and if there was any testing missing, I think it's been in production long enough 😆 )? Any outstanding issues (e.g. porting) can be logged/tracked via a separate issue...