435 basic list

[rolfheij-sil]

This change is 

[tjcouch-sil]

Reviewable status: 0 of 6 files reviewed, all discussions resolved

---

src/renderer/components/basic-list/basic-list.component.tsx line 10 at r1 (raw file):

  ColumnDef,
  flexRender,
} from '@tanstack/react-table';

Any particular reason you didn't want to use the data grid component we already have?

[irahopkinson]

Thanks for your work on this. It would be helpful to have a screenshot in the PR description so I don't have to run up your branch.

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @rolfheij-sil)

---

src/renderer/components/basic-list/basic-list.component.tsx line 18 at r1 (raw file):

export const TAB_TYPE_BASIC_LIST = 'basic-list';

type checkResult = {

Style: types (and interfaces and classes) should always start with a capital letter, i.e. use PascalCase.

---

src/renderer/components/basic-list/basic-list.component.tsx line 26 at r1 (raw file):

};

const newCheckResult = (bookId: string): checkResult => {

For functions that have simple returns, use the simplified return syntax for arrow functions, i.e.:

const newCheckResult = (bookId: string): checkResult => ({
  book: Canon.bookIdToEnglishName(bookId),
  chapter: faker.number.int(40),
  verse: faker.number.int(40),
  issueDescription: 'Basic check issue description',
});

---

src/renderer/components/basic-list/basic-list.component.tsx line 35 at r1 (raw file):

};

export function makeMockCheckResults() {

BTW Does this need to be exported? It's not currently used anywhere outside of this module.

---

src/renderer/components/basic-list/basic-list.component.tsx line 36 at r1 (raw file):

export function makeMockCheckResults() {
  const { allBookIds } = Canon;

BTW If you used this more than once it might make sense to destructure this but even then I would still just derefence it below since it is then easy to read that you are getting allBookIds from Canon on the one line, i.e.:

return Canon.allBookIds.map((bookId) => {

This may be more of a personal style call so feel free to leave it as is.

[rolfheij-sil]

Thanks for the feedback! I'll make sure to add a screenshot next time round

Reviewable status: 5 of 6 files reviewed, 3 unresolved discussions (waiting on @irahopkinson)

---

src/renderer/components/basic-list/basic-list.component.tsx line 10 at r1 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Any particular reason you didn't want to use the data grid component we already have?

Because I thought it did not support expanding rows. But I just realized that it does (https://adazzle.github.io/react-data-grid/#/master-detail)
I'll try to do a small investigation into using that component

---

src/renderer/components/basic-list/basic-list.component.tsx line 18 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Style: types (and interfaces and classes) should always start with a capital letter, i.e. use PascalCase.

Done. Shouldn't eslint check for that?

---

src/renderer/components/basic-list/basic-list.component.tsx line 26 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

For functions that have simple returns, use the simplified return syntax for arrow functions, i.e.:

const newCheckResult = (bookId: string): checkResult => ({
  book: Canon.bookIdToEnglishName(bookId),
  chapter: faker.number.int(40),
  verse: faker.number.int(40),
  issueDescription: 'Basic check issue description',
});

Done.

---

src/renderer/components/basic-list/basic-list.component.tsx line 35 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW Does this need to be exported? It's not currently used anywhere outside of this module.

Yup, you're right. The creation of mock data was in a separate file, but I decided to merge the two. Forgot to remove the export though.

---

src/renderer/components/basic-list/basic-list.component.tsx line 36 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW If you used this more than once it might make sense to destructure this but even then I would still just derefence it below since it is then easy to read that you are getting allBookIds from Canon on the one line, i.e.:

return Canon.allBookIds.map((bookId) => {

This may be more of a personal style call so feel free to leave it as is.

Done.

[irahopkinson]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @rolfheij-sil)

---

src/renderer/components/basic-list/basic-list.component.tsx line 18 at r1 (raw file):

Previously, rolfheij-sil wrote…

Done. Shouldn't eslint check for that?

Yes, but I don't yet know of a rule to use for this.

---

src/renderer/components/basic-list/basic-list.component.tsx line 33 at r2 (raw file):

});

const makeMockCheckResults = () => {

BTW not a blocker but curious why you changed this to an arrow function? I tend to use arrow functions for smaller and/or simpler returns or if I need a named function that I'm passing around and definitley if I'm using this in it, otherwise my default is to use function. functions are hoisted in the scope so they tend to cause less issues around where they are declared than arrow functions.

[rolfheij-sil]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @irahopkinson)

---

src/renderer/components/basic-list/basic-list.component.tsx line 33 at r2 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW not a blocker but curious why you changed this to an arrow function? I tend to use arrow functions for smaller and/or simpler returns or if I need a named function that I'm passing around and definitley if I'm using this in it, otherwise my default is to use function. functions are hoisted in the scope so they tend to cause less issues around where they are declared than arrow functions.

I changed it because for consistency reasons. The newCheckResults was also an arrow function, and I thought it'd look more uniform if I changed this one too.
Thanks for sharing your reasons for when to (not) use arrow functions, that's helpful!

[irahopkinson]

Reviewable status: complete! all files reviewed, all discussions resolved

[katherinejensen00]

Reviewed 3 of 6 files at r1, 1 of 1 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved