Run basic checks stub

[jolierabideau]

• ChapterRangeSelection created in PAPI Components- used in many places in Paratext and could be useful to extensions.
• Exposed getChaptersForBook from PAPI Scripture utils
• BasicChecks component maps checks to Checkboxes
• BookSelection component offers two radio groups: selecting chapter range for the current book, or choosing new books to check.
• RunBasicChecksTab to display everything and collect data from each of the components for posting.

I commented out the other floating tabs while I was developing, but should we leave the project dialogs and extension toggle commented out so that we don't overwhelm the screen on first load?

---

This change is 

[katherinejensen00]

I can't remember for sure, but I think there was talk of potentially removing the project dropdown as that was mainly there in P9 because of a bad state you could get into. I think we are hoping to avoid that bad state in Paratext.Bible, but I am not totally sure. Either way, UX is redesigning this anyway so it probably isn't worth changing now as it will probably be changed in that design if a change is needed.

Thank you Jolie and Tom for your great work on this! It looks amazing and I know that was a struggle. I have a few minor questions and notes, but it is basically done. The components you created as part of this task will be really helpful.

Reviewed 2 of 6 files at r1, 1 of 2 files at r4, 13 of 13 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @jolierabideau)

---

lib/papi-components/dist/index.d.ts line 38 at r5 (raw file):

export declare function Button({ id, isDisabled, className, onClick, onContextMenu, children, }: ButtonProps): import("react/jsx-runtime").JSX.Element;
export declare enum LabelPosition {
	After = "after",

I am probably missing something (like this was just moved somewhere else), but why was this deleted?

---

lib/papi-components/src/chapter-range-selection.component.tsx line 5 at r5 (raw file):

import ComboBox, { ComboBoxLabelOption } from './combo-box.component';

export type ChapterRangeSelectionProps = {

I think this is going to be a really helpful component to have. Thanks for creating it!

---

src/renderer/components/run-basic-checks-dialog/book-selection.component.tsx line 7 at r5 (raw file):

import './book-selection.component.scss';

type BookSelectionProps = {

Thanks for creating this! We know this will come up a lot :)

---

src/renderer/components/run-basic-checks-dialog/run-basic-checks-tab.component.tsx line 113 at r5 (raw file):

        options={projectNameOptions}
      />
      {/* Should always be two columns? */}

Great question for UX. I wonder if this could eventually change based on the size of the window. I think what you have is great for now though and should not change as part of this case. It might be nice to throw this question in to the UX case for designing the Run Checks dialog

---

src/renderer/testing/test-layout.data.ts line 5 at r5 (raw file):

import { TAB_TYPE_ABOUT } from '@renderer/testing/about-panel.component';
import { TAB_TYPE_BUTTONS } from '@renderer/testing/test-buttons-panel.component';
// import { TAB_TYPE_QUICK_VERSE_HERESY } from '@renderer/testing/test-quick-verse-heresy-panel.component';

Should these stay commented out, or did you just do that temporarily?

[tjcouch-sil]

Thanks for all the hard work on this! Looking good!

Reviewed 2 of 6 files at r1, 1 of 2 files at r4, 13 of 13 files at r5, all commit messages.
Reviewable status: all files reviewed, 21 unresolved discussions (waiting on @jolierabideau and @katherinejensen00)

---

lib/papi-components/dist/index.d.ts line 38 at r5 (raw file):

Previously, katherinejensen00 wrote…

I am probably missing something (like this was just moved somewhere else), but why was this deleted?

This is an autogenerated file, and it was just moved down further. I was confused at first too haha

---

lib/papi-components/src/chapter-range-selection.component.tsx line 8 at r5 (raw file):

  startChapter: number;
  endChapter: number;
  onChangeStartChapter: (_event: SyntheticEvent<Element, Event>, value: unknown) => void;

Making these value parameters typed would be very helpful. I think they are just number type.

Do you need the underscore on _event? I recommend removing it if not.

---

lib/papi-components/src/chapter-range-selection.component.tsx line 18 at r5 (raw file):

}

export default function ChapterRangeSelection({

I suggest renaming this to ChapterRangeSelector to indicate its role is to allow people to select books. It kinda reads to me like it is just a display of a chapter range selection as is.

---

lib/papi-components/src/chapter-range-selection.component.tsx line 27 at r5 (raw file):

}: ChapterRangeSelectionProps) {
  let chapterNumberOptions: ChapterNumberOption[];
  const numberArray = Array.from({ length: chapterCount }, (_, index) => index + 1);

It is probably best to wrap this in a useMemo so you aren't creating a new array every render.

---

lib/papi-components/src/chapter-range-selection.component.tsx line 29 at r5 (raw file):

  const numberArray = Array.from({ length: chapterCount }, (_, index) => index + 1);

  const getChapterNumberOptions = () => {

Have you tried just using the numberArray as the options? I think the combo box might support a number array, but you would have to change combo-box.component.tsx's ComboBoxOption type to include number because TypeScript doesn't realize it can support it:

export type ComboBoxOption = string | number | ComboBoxLabelOption;

That would simplify this code a lot. You wouldn't need the let or this function. You wouldn't need currentStartChapter or currentEndChapter :)

---

lib/papi-components/src/index.ts line 12 at r5 (raw file):

export { default as RefSelector } from './ref-selector.component';
export type { ScrRefSelectorProps } from './ref-selector.component';
export { getChaptersForBook } from './scripture/scripture-util';

I wonder if we should create a subpath export that exports all of the scripture utils we have. Not right now. Would probably make things more organized. #240

---

src/renderer/components/docking/paranext-dock-layout.component.tsx line 63 at r5 (raw file):

  TAB_TYPE_RUN_BASIC_CHECKS,
  loadRunBasicChecksTab,
} from '../run-basic-checks-dialog/run-basic-checks-tab.component';

This needs to be an @aliased/... path

---

src/renderer/components/run-basic-checks-dialog/basic-checks.component.tsx line 68 at r5 (raw file):

type BasicCheckProps = {
  handleSelectChecks: (checkName: string) => void;

Should this be handleSelectCheck since it handles one check at a time?

---

src/renderer/components/run-basic-checks-dialog/book-selection.component.tsx line 7 at r5 (raw file):

import './book-selection.component.scss';

type BookSelectionProps = {

I wonder if it would be helpful to union with the Chapter change props. Like

type BookSelectionProps = ChapterRangeSelectionProps & {
  ...
}

Are you using all the chapter selection props here? Are you intending to have all of them and to pass them all down? Union would probably be a good fit if so such that you don't have to copy prop types around.

---

src/renderer/components/run-basic-checks-dialog/book-selection.component.tsx line 9 at r5 (raw file):

type BookSelectionProps = {
  useCurrentBook: boolean;
  toggleCurrentBook: () => void;

Could you add an argument to this function which is a boolean of the new value to make the job easier for parent components?

---

src/renderer/components/run-basic-checks-dialog/book-selection.component.tsx line 12 at r5 (raw file):

  currentBookNumber: number;
  selectedBooks: number[];
  handleSelectBooks: (bookNumber: number) => void;

Should this be handleSelectBook?

---

src/renderer/components/run-basic-checks-dialog/book-selection.component.tsx line 20 at r5 (raw file):

};

export default function BookSelection({

I suggest renaming this to BookSelector to indicate its role is to allow people to select books similar to with the chapter range selection.

---

src/renderer/components/run-basic-checks-dialog/book-selection.component.tsx line 21 at r5 (raw file):

export default function BookSelection({
  useCurrentBook,

Booleans should be status words. I suggest maybe shouldUseCurrentBook.

---

src/renderer/components/run-basic-checks-dialog/book-selection.component.tsx line 22 at r5 (raw file):

export default function BookSelection({
  useCurrentBook,
  toggleCurrentBook,

Could you rename this to tighten its association with its variable? toggleShouldUseCurrentBook

---

src/renderer/components/run-basic-checks-dialog/run-basic-checks-tab.component.tsx line 12 at r5 (raw file):

import { useMemo, useState } from 'react';
import settingsService from '@shared/services/settings.service';
import { fetchProjects } from '../project-dialogs/open-project-tab.component';

Need a few @aliased paths here :)

---

src/renderer/components/run-basic-checks-dialog/run-basic-checks-tab.component.tsx line 47 at r5 (raw file):

  const [selectedBooks, setSelectedBooks] = useState<number[]>([currentBookNumber]);
  const [selectedChecks, setSelectedChecks] = useState<{ [key: string]: boolean }>(() => {

Can you make this an array of check names? Then you can default it to [] and simplify a few places.

---

src/renderer/components/run-basic-checks-dialog/run-basic-checks-tab.component.tsx line 82 at r5 (raw file):

  const handleSelectStartChapter = (newStart: number) => {
    setStartChapter(newStart);
    if (newStart > endChapter) setEndChapter(newStart);

Seems like this logic should be in the chapter range selector component

---

src/renderer/components/run-basic-checks-dialog/run-basic-checks-tab.component.tsx line 87 at r5 (raw file):

  const handleSelectEndChapter = (newEnd: number) => {
    setEndChapter(newEnd);
    if (newEnd < startChapter) setStartChapter(newEnd);

Seems like this logic should be in the chapter range selector component

[katherinejensen00]

Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @jolierabideau)

---

lib/papi-components/dist/index.d.ts line 38 at r5 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

This is an autogenerated file, and it was just moved down further. I was confused at first too haha

That makes sense. Thanks!

[jolierabideau]

There was talk about removing the project dropdown, I left a comment about that being changed eventually. I can take it out if that is preferred! That would remove the need to fetchProjects() in this tab. Thanks for your reviews!

Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @katherinejensen00 and @tjcouch-sil)

---

lib/papi-components/src/chapter-range-selection.component.tsx line 8 at r5 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Making these value parameters typed would be very helpful. I think they are just number type.

Do you need the underscore on _event? I recommend removing it if not.

Done

---

lib/papi-components/src/chapter-range-selection.component.tsx line 18 at r5 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I suggest renaming this to ChapterRangeSelector to indicate its role is to allow people to select books. It kinda reads to me like it is just a display of a chapter range selection as is.

Done That makes sense!

---

lib/papi-components/src/chapter-range-selection.component.tsx line 27 at r5 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

It is probably best to wrap this in a useMemo so you aren't creating a new array every render.

Done.

---

lib/papi-components/src/chapter-range-selection.component.tsx line 29 at r5 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Have you tried just using the numberArray as the options? I think the combo box might support a number array, but you would have to change combo-box.component.tsx's ComboBoxOption type to include number because TypeScript doesn't realize it can support it:

export type ComboBoxOption = string | number | ComboBoxLabelOption;

That would simplify this code a lot. You wouldn't need the let or this function. You wouldn't need currentStartChapter or currentEndChapter :)

This makes a lot more sense! I switched it to number[] but I am troubleshooting the autocomplete warning "MUI: The getOptionLabel method of Autocomplete returned number (31) instead of a string for 31."

---

lib/papi-components/src/index.ts line 12 at r5 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I wonder if we should create a subpath export that exports all of the scripture utils we have. Not right now. Would probably make things more organized. #240

I did wonder if these utils should be exported and if there was a better way to do it. So is this fine for now? Or would you like me to go ahead and add them all individually until we get to this subpath issue?

---

src/renderer/components/docking/paranext-dock-layout.component.tsx line 63 at r5 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

This needs to be an @aliased/... path

Done.

---

src/renderer/components/run-basic-checks-dialog/basic-checks.component.tsx line 68 at r5 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Should this be handleSelectCheck since it handles one check at a time?

Done.

---

src/renderer/components/run-basic-checks-dialog/book-selection.component.tsx line 7 at r5 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

I wonder if it would be helpful to union with the Chapter change props. Like

type BookSelectionProps = ChapterRangeSelectionProps & {
  ...
}

Are you using all the chapter selection props here? Are you intending to have all of them and to pass them all down? Union would probably be a good fit if so such that you don't have to copy prop types around.

I am using startChapter, endChapter, and chapterCount. So I omitted the onChangeStart and onChangeEnd.

---

src/renderer/components/run-basic-checks-dialog/book-selection.component.tsx line 9 at r5 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Could you add an argument to this function which is a boolean of the new value to make the job easier for parent components?

Done

---

src/renderer/components/run-basic-checks-dialog/book-selection.component.tsx line 12 at r5 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Should this be handleSelectBook?

It was stubbed to only select one book, but really users will be able to select multiple so I turned that into an array of numbers.

---

src/renderer/components/run-basic-checks-dialog/book-selection.component.tsx line 21 at r5 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Booleans should be status words. I suggest maybe shouldUseCurrentBook.

Done.

---

src/renderer/components/run-basic-checks-dialog/book-selection.component.tsx line 22 at r5 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Could you rename this to tighten its association with its variable? toggleShouldUseCurrentBook

Done.

---

src/renderer/components/run-basic-checks-dialog/run-basic-checks-tab.component.tsx line 12 at r5 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Need a few @aliased paths here :)

Done.

---

src/renderer/components/run-basic-checks-dialog/run-basic-checks-tab.component.tsx line 47 at r5 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Can you make this an array of check names? Then you can default it to [] and simplify a few places.

Done.

---

src/renderer/components/run-basic-checks-dialog/run-basic-checks-tab.component.tsx line 82 at r5 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Seems like this logic should be in the chapter range selector component

That is a good point. I am trying to figure out how best to accomplish this without passing the setEndChapter as a prop and while keeping the state variable in the tab so that I can collect the start and end.

---

src/renderer/components/run-basic-checks-dialog/run-basic-checks-tab.component.tsx line 87 at r5 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Seems like this logic should be in the chapter range selector component

That is a good point. I am trying to figure out how best to accomplish this without passing the setEndChapter as a prop and while keeping the state variable in the tab so that I can collect the start and end.

---

src/renderer/components/run-basic-checks-dialog/run-basic-checks-tab.component.tsx line 113 at r5 (raw file):

Previously, katherinejensen00 wrote…

Great question for UX. I wonder if this could eventually change based on the size of the window. I think what you have is great for now though and should not change as part of this case. It might be nice to throw this question in to the UX case for designing the Run Checks dialog

I attempted to switch to one when the window wasn't wide enough for two. But it became difficult with so many unknowns for width of tab and how much space each section should take up. I can add it to the UX case!

---

src/renderer/testing/test-layout.data.ts line 5 at r5 (raw file):

Previously, katherinejensen00 wrote…

Should these stay commented out, or did you just do that temporarily?

I did do that while I was developing. But, now that we have so many floating tabs I am wondering if it makes sense to leave some of them commented out? I could leave Quick Verse and then we wouldn't have to clear out of all the tabs on a fresh start.

[katherinejensen00]

Reviewed 15 of 15 files at r6, all commit messages.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @tjcouch-sil)

---

src/renderer/components/run-basic-checks-dialog/run-basic-checks-tab.component.tsx line 113 at r5 (raw file):

Previously, jolierabideau wrote…

I attempted to switch to one when the window wasn't wide enough for two. But it became difficult with so many unknowns for width of tab and how much space each section should take up. I can add it to the UX case!

Great, thanks, Jolie!

---

src/renderer/testing/test-layout.data.ts line 5 at r5 (raw file):

Previously, jolierabideau wrote…

I did do that while I was developing. But, now that we have so many floating tabs I am wondering if it makes sense to leave some of them commented out? I could leave Quick Verse and then we wouldn't have to clear out of all the tabs on a fresh start.

That makes sense. Thanks for explaining that!

[tjcouch-sil]

just one question (potentially with action or potentially not) and two tiny things to ignore for now unless you end up making other changes. Thanks for all the hard work! Great stuff! If you don't need to make any changes, feel free to resolve my comments and get this in.

Reviewed 14 of 15 files at r6, 9 of 9 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @jolierabideau)

---

lib/papi-components/src/index.ts line 12 at r5 (raw file):

Previously, jolierabideau wrote…

I did wonder if these utils should be exported and if there was a better way to do it. So is this fine for now? Or would you like me to go ahead and add them all individually until we get to this subpath issue?

Yep, just fine for now. I just put this here to have a link to it for later. I put the status of this discussion as "Informing" (the i in a circle) to indicate there's no action for now :) thanks for checking!

---

src/renderer/components/run-basic-checks-dialog/book-selector.component.tsx line 7 at r6 (raw file):

import './book-selector.component.scss';

type BookSelectionProps = Omit<

No big deal, but this is still named Selection. No need to change this now unless you end up making other changes.

---

src/renderer/components/run-basic-checks-dialog/run-basic-checks-tab.component.tsx line 38 at r6 (raw file):

  const [useCurrentBook, setUseCurrentBook] = useState<boolean>(true);

  const handleSelectBooks = (bookNumbers: number[]) => {

Previously, this handled deselecting books, but it doesn't seem to now. Is it still intended to do so, or not anymore? If not right now, feel free to resolve this comment. If so, maybe the argument bookNumbers can be an object whose keys are books and values are true or false? Just a suggestion! :) Do what makes sense to you!

---

lib/papi-components/src/chapter-range-selector.component.tsx line 33 at r6 (raw file):

        disabled={isDisabled}
        control={
          <ComboBox

ComboBox has a generic type. In order to avoid type asserting in onChange, you should be able to change this line to <ComboBox` (and line 50). Not a big deal this time; no need to change unless you're already making other changes

[jolierabideau]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tjcouch-sil)

---

src/renderer/components/run-basic-checks-dialog/run-basic-checks-tab.component.tsx line 38 at r6 (raw file):

Previously, tjcouch-sil (TJ Couch) wrote…

Previously, this handled deselecting books, but it doesn't seem to now. Is it still intended to do so, or not anymore? If not right now, feel free to resolve this comment. If so, maybe the argument bookNumbers can be an object whose keys are books and values are true or false? Just a suggestion! :) Do what makes sense to you!

I believe it does deselect books with line 52.

[tjcouch-sil]

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

---

src/renderer/components/run-basic-checks-dialog/run-basic-checks-tab.component.tsx line 38 at r6 (raw file):

Previously, jolierabideau wrote…

I believe it does deselect books with line 52.

Oh cool, thanks! Sorry, looks like I wrote that comment on the second-to-last revision, then I didn't notice the change when I updated to the latest revision.