PT-3662, PT-3744, partially PT-3661 by Sebastian-ubs · Pull Request #1949 · paranext/paranext-core

Sebastian-ubs · 2025-12-10T10:39:17Z

Now depends on #1952

This PR changes the result card click behavior to match Interaction design

single click or space/enter on the card = select item
double click or click/space/enter on the button = change reference, focus editor + select text

This PR fixes / helps for the following work items

fixes PT-3662 → scripture reference as button
(this also mitigates PT-3412 → no unexpected focus change on single click)
fixes PT-3661 → result styling (partially) - not just cutting off text
fixes PT-3744 → truncate find result

This change is

tjcouch-sil

If they are still relevant after changes made, please fix the TypeDoc warnings (identified in your top-level message as "build warnings"). We expect there to be none (and may be soon requiring there to be none).

@tjcouch-sil reviewed 11 of 17 files at r1, all commit messages.
Reviewable status: 11 of 17 files reviewed, 7 unresolved discussions (waiting on @Sebastian-ubs)

lib/platform-bible-react/src/utils/dir-helper.util.ts line 18 at r1 (raw file):

  if (!hasLocalStorage()) return 'ltr';

  try {

try/catch around the contents of readDirection and persistDirection as they were written before sounds good to avoid blowing things up, but I think we should still warn with the error message that comes from accessing localStorage.getItem when localStorage is not defined. I don't think we should check for if window is defined and only use window.localStorage, though, because localStorage should be available in this context as far as I know. It seems this is a bandage that covers a problem rather than fixing it.

I'd rather see this continue to warn (or at least log in some way) when it fails and have a work item to look into it rather than to have it silently fail and we are confused as to why something isn't matching the direction properly later on when we start looking more closely at RTL.

All the same goes for the code in the storybook theme provider file, but I don't care about that one very much.

lib/platform-bible-react/src/components/basics/scripture-reference-button.component.tsx line 10 at r1 (raw file):

export type ScrRefBtnProps = {
  startRef: ScrRef;

Can we use SerializedVerseRef instead of this new ScrRef? We have pretty much unified around SerializedVerseRef in all our dealings with verse/Scripture references. I would like to avoid adding a new type that muddies the waters.

Presumably, at some point, we will make some class that formats Verse Refs in a project-aware way. Once we have that, it would be best to have SerializedVerseRef here anyway so we can pass that into the formatter.

lib/platform-bible-react/src/components/basics/scripture-reference-button.component.tsx line 14 at r1 (raw file):

  text?: string;
  className?: string;
  onClick?: (e: unknown) => void;

onClick's type should be MouseEventHandler<T> | undefined

lib/platform-bible-react/src/components/basics/scripture-reference-button.component.tsx line 15 at r1 (raw file):

  className?: string;
  onClick?: (e: unknown) => void;
  remainderTextLength?: number;

This is not self-explanatory. If this code stays around, please add TSDoc. Also, I would recommend probably renaming it. It doesn't seem like it from the variable name, but I believe I see this number applies to how many words at the start and the end of the text to keep, but I generally expect characters when I see "text length".

lib/platform-bible-react/src/components/basics/scripture-reference-button.component.tsx line 18 at r1 (raw file):

};

function trimMiddleWordsWithRemovedCount(sentence: string, x: number): string {

The function name is not very clear to me; might I recommend something like truncateStringByOmittingMiddleWords, replaceMiddleWordsWithEllipsis, shortenStringOmitMiddleWords, or something. "Trim" brings cutting off the ends to mind like the built-in trim functions, and "with removed count" made me think you were returning how many words you were taking out from the middle.

These parameter names do not make sense. Please adjust. And maybe should this stuff go in string utils in PBU?

lib/platform-bible-react/src/components/basics/scripture-reference-button.component.tsx line 32 at r1 (raw file):

}

function ScrRefButton({

Honestly I really don't think this makes sense as a generic component to be exported from PBR. It's used one time, which is only inside PBR. It doesn't really make clear sense from the outside how the different pieces are combined to create the button (for example, I was expecting this button simply to display a SerializedVerseRef or a range of them, but then I saw there was also text. It's not intuitive what text is for. I'd recommend at least not exporting this from PBR for now so we have more time to figure out what a more natural API would be for such a component.

I also recommend either to in-line this in ResultsCard or to make it more self-explanatory in some way. It doesn't really make sense to me to combine the functionality of a button that is intended to display a range of verse references with the functionality of a button that is intended to display some text that should be truncated in some very particular way in some situations. For example, should the check result text really be truncated when it gets past 14 words? I wouldn't expect so. Maybe can these two different sets of functionality be split into two different components? Maybe you pass in the truncated one into the button via children? Or maybe make a VerseRefDisplay component that just handles displaying the verse ref in some way, then you can put these two together under a normal button? Or maybe that's breaking it into too small of pieces to be worth doing, so maybe in-lining is the right call. I dunno what exactly the best solution is, but this button seems very strange.

lib/platform-bible-react/src/components/basics/scripture-reference-button.component.tsx line 50 at r1 (raw file):

        aria-label="Submit reference change"
        onClick={(e) => {
          e.stopPropagation(); // Stops bubbling to outer div

I'd argue it's better to just directly call onClick as it is passed in rather than to stop propagation here since this a standard HTML event. If someone wants to stop propagation, they can by running it on the event that is passed in as the first parameter.

tjcouch-sil

Thanks for tackling all this stuff! The lists look nicer.

For the sake of my own curiosity, do you know how double clicking works on keyboard? Maybe it doesn't and you have to think of other ways to do double clicking actions on keyboard; just curious.

Reviewable status: 11 of 17 files reviewed, 7 unresolved discussions (waiting on @Sebastian-ubs)

rolfheij-sil

@rolfheij-sil reviewed 15 of 17 files at r1, all commit messages.
Reviewable status: 16 of 17 files reviewed, 11 unresolved discussions (waiting on @Sebastian-ubs)

lib/platform-bible-react/src/index.ts line 212 at r1 (raw file):

} from './components/shadcn-ui/tooltip';
export type { Scope } from './components/utils/scripture.util';
export { default as ScrRefButton } from './components/basics/scripture-reference-button.component';

Can you move this to the basics exports section?
So alphabetically it should go between these items:

export { default as ResultsCard } from './components/basics/results-card.component';
export { default as SearchBar } from './components/basics/search-bar.component';

While you're at it, would you mind moving this export to that section too?

export {
  VerticalTabs,
  VerticalTabsList,
  VerticalTabsContent,
  VerticalTabsTrigger,
} from './components/basics/tabs-vertical';

extensions/src/platform-scripture/src/checks/checks-side-panel/check-card.component.tsx line 81 at r1 (raw file):

  handleDenyCheck: (result: CheckRunResult) => Promise<boolean>;
  /** Scripture reference as link */
  scrRef: ScrRefBtnProps;

I think scrRef functions as a somewhat protected keyword in our codebase I think. We will exclusively using to actually store scripture references using the SerialzedVerseRef type. It's confusing that it's now used from a set of props that contains some scripture references.

Moreover, it's interesting that we're nesting props inside props here. (ScrRefBtnProps inside of CheckCardProps). That might be a code smell.

lib/platform-bible-react/src/components/basics/results-card.component.tsx line 61 at r1 (raw file):

    // eslint-disable-next-line no-type-assertion/no-type-assertion
    const activeEl = document.activeElement as HTMLElement;
    const isButtonFocused = activeEl?.closest('button') || activeEl?.tagName === 'BUTTON';

What are you trying to achieve here? This will return true if any button is focused right, not specifically a button inside the card, right?
Can't we handle this with a useRef to make ik more reliable and specific?

lib/platform-bible-react/src/components/basics/results-card.component.tsx line 80 at r1 (raw file):

      onMouseDown={(e) => {
        // cancel double‑click text selection
        if (e.detail > 1) e.preventDefault();

Why add the if statement? Or why add the onMouseDown at all? Won't the default already be to do nothing when on mouse down if we don't provide a handler?

Sebastian-ubs

Reviewable status: 2 of 18 files reviewed, 11 unresolved discussions (waiting on @rolfheij-sil and @tjcouch-sil)

extensions/src/platform-scripture/src/checks/checks-side-panel/check-card.component.tsx line 81 at r1 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

I think scrRef functions as a somewhat protected keyword in our codebase I think. We will exclusively using to actually store scripture references using the SerialzedVerseRef type. It's confusing that it's now used from a set of props that contains some scripture references.

Moreover, it's interesting that we're nesting props inside props here. (ScrRefBtnProps inside of CheckCardProps). That might be a code smell.

Done updated ref to use SerializedVerseRef
Nesting props imho is actually a good thing to keep the interface clear, but we may argue about it :-)

lib/platform-bible-react/src/index.ts line 212 at r1 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

Can you move this to the basics exports section?
So alphabetically it should go between these items:
export { default as ResultsCard } from './components/basics/results-card.component';
export { default as SearchBar } from './components/basics/search-bar.component';
While you're at it, would you mind moving this export to that section too?
export {
  VerticalTabs,
  VerticalTabsList,
  VerticalTabsContent,
  VerticalTabsTrigger,
} from './components/basics/tabs-vertical';

Done.

lib/platform-bible-react/src/components/basics/results-card.component.tsx line 80 at r1 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

Why add the if statement? Or why add the onMouseDown at all? Won't the default already be to do nothing when on mouse down if we don't provide a handler?

This is to prevent double click on the card ('s text) to make a text selection. Otherwise the Electron default for double clicking is to always do a text selection of the word under the cursor. I had looked for a solution to this earlier for Home and plan to also apply it there.

lib/platform-bible-react/src/components/basics/scripture-reference-button.component.tsx line 10 at r1 (raw file):