[Review Entries] Refactor table with material-react-table

[imnasnainaec]

Resolves #2875

Requires #2876

---

This change is 

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 76.90909% with 127 lines in your changes are missing coverage. Please review.

Project coverage is 74.34%. Comparing base (94f89f9) to head (eafc91c).

Files	Patch %	Lines
...s/ReviewEntriesTable/Cells/EditCell/EditDialog.tsx	63.79%	38 Missing and 4 partials ⚠️
...iewEntriesTable/Cells/EditCell/EditSenseDialog.tsx	67.70%	28 Missing and 3 partials ⚠️
...c/goals/ReviewEntries/ReviewEntriesTable/index.tsx	82.14%	14 Missing and 1 partial ⚠️
...es/ReviewEntriesTable/Cells/PronunciationsCell.tsx	33.33%	12 Missing ⚠️
...riesTable/Cells/EditCell/EditSensesCardContent.tsx	73.91%	6 Missing ⚠️
...iewEntries/ReviewEntriesTable/Cells/DeleteCell.tsx	63.63%	4 Missing ⚠️
...ntries/ReviewEntriesTable/Cells/EditCell/index.tsx	55.55%	4 Missing ⚠️
...ewEntries/ReviewEntriesTable/Cells/GlossesCell.tsx	75.00%	2 Missing and 2 partials ⚠️
...tries/ReviewEntriesTable/Cells/DefinitionsCell.tsx	87.50%	1 Missing and 1 partial ⚠️
...ies/ReviewEntriesTable/Cells/EditCell/utilities.ts	96.77%	1 Missing and 1 partial ⚠️
... and 3 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2881      +/-   ##
==========================================
+ Coverage   73.46%   74.34%   +0.88%     
==========================================
  Files         268      269       +1     
  Lines       10306    10321      +15     
  Branches     1212     1217       +5     
==========================================
+ Hits         7571     7673     +102     
+ Misses       2382     2299      -83     
+ Partials      353      349       -4     

Flag	Coverage Δ
backend	83.05% <ø> (+0.10%)	⬆️
frontend	66.30% <76.90%> (+1.62%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jasonleenaylor]

src/goals/ReviewEntries/ReviewEntriesTable/index.tsx line 36 at r4 (raw file):

  switch (lang) {
    case "ar":
      return (await import("material-react-table/locales/ar"))

Consider if we can or want to use i18next getResource here just to keep our localizations all in one place and usable by crowdin.

[imnasnainaec]

Reviewable status: 0 of 86 files reviewed, all discussions resolved (waiting on @jasonleenaylor)

---

src/goals/ReviewEntries/ReviewEntriesTable/index.tsx line 36 at r4 (raw file):

Previously, jasonleenaylor (Jason Naylor) wrote…

Consider if we can or want to use i18next getResource here just to keep our localizations all in one place and usable by crowdin.

The way material-react-table uses localization data (e.g., https://github.com/KevinVandy/material-react-table/blob/v2/packages/material-react-table/src/components/menus/MRT_RowActionMenu.tsx#L62), it seems that if we did load their localizations into i18n, we'd then have to extract them from i18n to manually feed into the table's localization prop.

[jasonleenaylor]

Reviewed 56 of 85 files at r1, 15 of 22 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: 71 of 86 files reviewed, all discussions resolved (waiting on @imnasnainaec)

---

src/goals/ReviewEntries/ReviewEntriesTable/Cells/EditCell/EditDialog.tsx line 135 at r4 (raw file):

  const [showSenses, setShowSenses] = useState(true);

  useEffect(() => {

There are some pros and cons to doing this here, I think I like it though. The con is a possible delay between the user change and the state updating, make sure there is no race condition on a keyboard change and accept.

[jasonleenaylor]

src/goals/ReviewEntries/ReviewEntriesTable/index.tsx line 36 at r4 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

The way material-react-table uses localization data (e.g., https://github.com/KevinVandy/material-react-table/blob/v2/packages/material-react-table/src/components/menus/MRT_RowActionMenu.tsx#L62), it seems that if we did load their localizations into i18n, we'd then have to extract them from i18n to manually feed into the table's localization prop.

Yes, that is what I was seeing as well. It seems that what you have here is probably the cleanest choice.

[jasonleenaylor]

src/goals/ReviewEntries/ReviewEntriesTable/Cells/EditCell/EditSenseDialog.tsx line 143 at r4 (raw file):

    const cleanedSense = cleanSense(newSense);
    if (!cleanedSense || typeof cleanedSense === "string") {
      toast.error(t(cleanedSense ?? ""));

I'm not sure I like the multiple return type approach.
Another option is to have cleanSense take two functions, one to execute on error and one to execute on success.

There are probably others. The current choice has us checking conditions twice, checking the state to choose a return type and checking the return type to determine the action.

[jasonleenaylor]

Reviewed 7 of 85 files at r1, 7 of 22 files at r2, 1 of 1 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @imnasnainaec)

---

src/goals/ReviewEntries/ReviewEntriesTable/Cells/EditCell/index.tsx line 10 at r4 (raw file):

const buttonId = (wordId: string): string => `row-${wordId}-edit`;

export default function DeleteCell(props: CellProps): ReactElement {

So, DeleteCell returns an Edit cell...

[imnasnainaec]

Reviewable status: 85 of 86 files reviewed, 2 unresolved discussions (waiting on @jasonleenaylor)

---

src/goals/ReviewEntries/ReviewEntriesTable/Cells/EditCell/EditSenseDialog.tsx line 143 at r4 (raw file):

Previously, jasonleenaylor (Jason Naylor) wrote…

I'm not sure I like the multiple return type approach.
Another option is to have cleanSense take two functions, one to execute on error and one to execute on success.

There are probably others. The current choice has us checking conditions twice, checking the state to choose a return type and checking the return type to determine the action.

This logic was carried over from the old functions, but I'll look into updating it.

---

src/goals/ReviewEntries/ReviewEntriesTable/Cells/EditCell/index.tsx line 10 at r4 (raw file):

Previously, jasonleenaylor (Jason Naylor) wrote…

So, DeleteCell returns an Edit cell...

Done.

[imnasnainaec]

Reviewable status: 85 of 86 files reviewed, 2 unresolved discussions (waiting on @jasonleenaylor)

---

src/goals/ReviewEntries/ReviewEntriesTable/Cells/EditCell/EditDialog.tsx line 135 at r4 (raw file):

Previously, jasonleenaylor (Jason Naylor) wrote…

There are some pros and cons to doing this here, I think I like it though. The con is a possible delay between the user change and the state updating, make sure there is no race condition on a keyboard change and accept.

Oh, good consideration! To accept changes with the keyboard, one has to tab to the check button and then press Enter. With no async updates in this dialog, I don't see keyboard race conditions as an issue.

[jasonleenaylor]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @imnasnainaec)

---

src/goals/ReviewEntries/ReviewEntriesTable/Cells/EditCell/EditSenseDialog.tsx line 143 at r4 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

This logic was carried over from the old functions, but I'll look into updating it.

This can be handled in a follow up PR since it was pre-existing code.