add proj-res filter to Home

[Sebastian-ubs]

fixes PT-3182 Filter Home by resource type or project
Also

• Distinguish read-only from editable projects from resources in Home and New Tab - also without internet connection
• Have a tooltip on the project short name & icon
• Have a tooltip on the modification date
• Improved footer summary now indicating number of filtered items

outdated screenshots

and

outdated tasks

Tasks

• set type when resources are loaded (partially done - cannot do more without backend changes)
• fix errors
• check if imports from pb-utils from within pbr is ok
• build and run Platform to see if it also works there
• move types to pbr, remove from platform-scripture and re-export them there to not break the interface

For a different PR

• move the ProjectTypes to paranext-core (next to home webview) to allow them to be dynamic
  (also allows to test in pbr with fantasy types, whereas platform.bible only uses the types it really provides)
  • start to use condition and default for the actions defined by a type to set the available buttons
  • add localization for the new strings (name of types) & existing strings (name of actions, but now used inside ProjectTypes)
  • do not require the platform-scripture send-receive types, but only a more generic type for platform projects and the fields they should offer in the home table

You can see the Home with the filter in platform-bible-react or in Platform.Bible

This change is 

---

[Sebastian-ubs]

ready for review

[lyonsil]

I'm pausing my review partway through because I'm a bit confused about why so much code is moving out of a WeVew and into PBR when it's not really a reusable component. From a PR standpoint, the code move also makes it difficult to see what changed specifically to fix PT-3006 and PT-3182.

Could you keep the dialog code where it was and have a separate discussion about moving most home logic out of the dialog and into a PBR component? If you really think it belongs in PBR, let's talk about it tomorrow.

Reviewed 3 of 22 files at r1, 13 of 20 files at r3, all commit messages.
Reviewable status: 16 of 23 files reviewed, 1 unresolved discussion

---

lib/platform-bible-react/src/components/advanced/filterable-resource-list/home.component.tsx line 0 at r3 (raw file):
Why are we moving a WebView into the components section of PBR? Home is a WebView, not really a reusable component (which is what I understand PBR/src/components to be there to provide). Even if it does belong in PBR, it doesn't make sense to me that Home belongs in the folder filterable-resource-list. That folder that was created for another component (filter.component.tsx).

Beyond these two points, I assume there will need to be some reworking on this WebView to incorporate the MIME-type idea or something along those lines. I worry that moving it here might make that process more complicated as this will force continued rebuilding of PBR to see updates in the WebView. Normally I would expect generic components to live in PBR that have meaning and individual testability. I would think this component could only be fully tested and updated by building PBR then rerunning in the application.

[Sebastian-ubs]

Thank you Matt for giving it a try. I had previously discussed this approach with TJ and Rolf. Rolf is obviously not available and TJ maybe was busy otherwise, not sure.
Sorry for mixing splitting with the implementation of these two changes. I agree this makes it harder.
For reasons why to split / move, see the other comment...

Reviewable status: 16 of 23 files reviewed, 1 unresolved discussion (waiting on @lyonsil)

---

lib/platform-bible-react/src/components/advanced/filterable-resource-list/home.component.tsx line at r3 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Why are we moving a WebView into the components section of PBR? Home is a WebView, not really a reusable component (which is what I understand PBR/src/components to be there to provide). Even if it does belong in PBR, it doesn't make sense to me that Home belongs in the folder filterable-resource-list. That folder that was created for another component (filter.component.tsx).

Beyond these two points, I assume there will need to be some reworking on this WebView to incorporate the MIME-type idea or something along those lines. I worry that moving it here might make that process more complicated as this will force continued rebuilding of PBR to see updates in the WebView. Normally I would expect generic components to live in PBR that have meaning and individual testability. I would think this component could only be fully tested and updated by building PBR then rerunning in the application.

In my understanding pbr is not only a place for shared components, but also a place to develop components / uis independent of the backend logic. In opposite to your expectation, I think doing changes in home will be made much simpler like this. Reasons to this:

• home was a big block of 600 lines of code and hard to find where papi calls are happening vs when data is processed for presentation vs display logic. Splitting it, enables to further refactor and split out the actual reusable parts that are theoretically shared between home and get resources, but for now are a copy of that code
• with having home in pbr you can fix sorting, ui controls, width, overflow, colors, filtering, loading logic, ... without having to run platform.bible, also new ui controls can be easily introduced (as long as they do not require additional backend logic - see below)
• with having home in pbr you can develop ui changes even against a yet-to-come interface, even when the logic for this does not yet exists in core. Of course this will only start to work, when this interface will actually be implemented in the backend. But will e.g. enable to implement MIME type handling from a ui perspective, even when the home webview does not yet offer this
• lastly it enables to stress test the ui with a lot of responses / states and data, that you will not all have available as test data in a running platform.bible application

For the folder you are right, that place seems not to be the best. Maybe there should be another folder in components for whole uis?
Other uis that currently are also in pbr are

• inventory table - which was very helpful e.g. to test and implement keyboard navigation in the table
• marketplace - which will be very helpful to do the upcoming marketplace fixes
• I'm also eager to see the checks side panel added to pbr
  • it would have already helped with some fixes
  • it will be helpful when more functionality will be added
  • this (or at least large parts of it) are intended to be reused for Find/Replace and Notes.

[Sebastian-ubs]

Reviewable status: 16 of 23 files reviewed, 1 unresolved discussion (waiting on @lyonsil)

---

lib/platform-bible-react/src/components/advanced/filterable-resource-list/home.component.tsx line at r3 (raw file):

Previously, Sebastian-ubs wrote…

In my understanding pbr is not only a place for shared components, but also a place to develop components / uis independent of the backend logic. In opposite to your expectation, I think doing changes in home will be made much simpler like this. Reasons to this:

• home was a big block of 600 lines of code and hard to find where papi calls are happening vs when data is processed for presentation vs display logic. Splitting it, enables to further refactor and split out the actual reusable parts that are theoretically shared between home and get resources, but for now are a copy of that code
• with having home in pbr you can fix sorting, ui controls, width, overflow, colors, filtering, loading logic, ... without having to run platform.bible, also new ui controls can be easily introduced (as long as they do not require additional backend logic - see below)
• with having home in pbr you can develop ui changes even against a yet-to-come interface, even when the logic for this does not yet exists in core. Of course this will only start to work, when this interface will actually be implemented in the backend. But will e.g. enable to implement MIME type handling from a ui perspective, even when the home webview does not yet offer this
• lastly it enables to stress test the ui with a lot of responses / states and data, that you will not all have available as test data in a running platform.bible application

For the folder you are right, that place seems not to be the best. Maybe there should be another folder in components for whole uis?
Other uis that currently are also in pbr are

• inventory table - which was very helpful e.g. to test and implement keyboard navigation in the table
• marketplace - which will be very helpful to do the upcoming marketplace fixes
• I'm also eager to see the checks side panel added to pbr
  • it would have already helped with some fixes
  • it will be helpful when more functionality will be added
  • this (or at least large parts of it) are intended to be reused for Find/Replace and Notes.

sorry, starting the ux review of marketplace, I just recognized that the marketplace ui itself is not in pbr, which makes it hard to test and fix, because I only see 1 real extension in platform.bible, so I cannot test a lot of different states next to each other.

[Sebastian-ubs]

As discussed with Matt, we are pausing this review until after the next ux+dev meeting to discuss general approach of whole uis inside pbr

Reviewable status: 16 of 23 files reviewed, 1 unresolved discussion (waiting on @lyonsil)

[Sebastian-ubs]

ready for another review.
Now Home.component is in platform-get-resources (bundled extension) - in line with agreement of last ux-dev meeting

[Sebastian-ubs]

in npm run start

[lyonsil]

Thanks for the updates, Sebastian. I added several comments about the new types added. My biggest concern is the overloading of the word "project" to mean both "a distinct collection of data regardless of what kind of data that is" and "a project as Paratext 9 knows a project (e.g., a scripture translation)." I think we need to differentiate between the two meanings in how we name things or it's going to be very confusing in the code to know what we're working with.

I'm not commenting directly on the home component changes as so many lines moved between files that I'm not sure what changed and if it looks good/better as-is. I think @rolfheij-sil would probably be the best person to comment on that as he wrote the original WebView for this.

Separately, I didn't look closely at the stories as I'm not familiar with Storybook stories. I thought it would be better for someone with experience with those to comment.

Reviewed 15 of 24 files at r4, 5 of 9 files at r5, all commit messages.
Reviewable status: 21 of 26 files reviewed, 8 unresolved discussions

---

extensions/src/platform-get-resources/src/types/project-type.d.ts line 4 at r5 (raw file):

import { ComponentType, SVGProps } from 'react';

export type LocalProjectInfo = {

I think Project here means "a distinct collection of data regardless of what kind of data that is."

---

extensions/src/platform-get-resources/src/types/project-type.d.ts line 4 at r5 (raw file):

import { ComponentType, SVGProps } from 'react';

export type LocalProjectInfo = {

All of our exported types should include TSDOC strings.

---

extensions/src/platform-get-resources/src/types/project-type.d.ts line 9 at r5 (raw file):

  fullName: string;
  name: string;
  language: string;

We probably need to add comments here on which kind of formatting this is supposed to contain (e.g., BCP-47, ISO-639-1, ISO-639-2, ISO 639-3, etc.).

---

extensions/src/platform-get-resources/src/types/project-type.d.ts line 13 at r5 (raw file):

};

export type MergedProjectInfo = {

I think Project here means "a project as Paratext 9 knows a project (e.g., a scripture translation)."

---

extensions/src/platform-get-resources/src/types/project-type.d.ts line 22 at r5 (raw file):

  isLocallyAvailable?: boolean;
  editedStatus?: EditedStatus;
  lastSendReceiveDate?: string;

Why not use Date here instead of string?

---

extensions/src/platform-get-resources/src/types/project-type.d.ts line 26 at r5 (raw file):

};

export const PROJECT_TYPE_KEYS: readonly ['project', 'resource', 'dictionary', 'media'];

I find myself a bit confused on one type of project being project while other types of projects are not project. It feels like we're overloading the term here. Do we mean "project" as in "a distinct collection of data to view and possibly edit?" or "a project as Paratext 9 knows a project"? Or maybe something else?

Perhaps the key should be renamed to paratext9 (or something very similar) so that "project" means "a distinct collection of data". If we don't want to do that, then I would suggest need a new meaning for "a distinct collection of data" regardless of whether it's an editable translation, a resource, a dictionary, media, etc.

---

extensions/src/platform-get-resources/src/types/project-type.d.ts line 28 at r5 (raw file):

export const PROJECT_TYPE_KEYS: readonly ['project', 'resource', 'dictionary', 'media'];
export type ProjectTypeKey = (typeof PROJECT_TYPE_KEYS)[number];
export type TypeAction = {

At first glance this type is a little confusing based on its name alone. I think I realize what this is intended to represent now after some thought, but adding TSDOCs and renaming to something like ProjectTypeAction would help future reviewers.

---

extensions/src/platform-get-resources/src/types/project-type.d.ts line 31 at r5 (raw file):

  buttonLabel: string;
  action: () => void;
  condition: () => boolean;

I don't understand the purpose of condition, and examples in project-types.ts all return true. If the idea is whether type action can apply to a particular project, I would expect there to be an input argument to represent the project. And really I would expect the same argument type(s) to be passed to action and default.

---

extensions/src/platform-get-resources/src/types/project-type.d.ts line 36 at r5 (raw file):

export type ProjectType = {
  key: ProjectTypeKey;
  localizedName: string;

This implies that the values are localized, but we are hard-coding English values in our types. It would probably be better to specify the localization key in this type and let the UI look up the actual localized name.

---

extensions/src/platform-get-resources/src/types/project-types.ts line 7 at r5 (raw file):

  project: {
    key: 'project',
    localizedName: 'Projects',

Just noting my object here again to using project to mean both a generic kind of thing we open and a specific kind of scripture-related thing with a set of files that PT9 understands and we inherit through ParatextData.dll.

[rolfheij-sil]

Thanks for doing all this Sebastian. Must have been a lot of work. I took a good look at all the TSX / front-end code.

Reviewed 1 of 22 files at r1, 16 of 24 files at r4, 9 of 9 files at r5, all commit messages.
Reviewable status: all files reviewed, 25 unresolved discussions (waiting on @Sebastian-ubs)

---

lib/platform-bible-react/src/preview/pages/components/advanced.component.tsx line 81 at r5 (raw file):

          <VerticalTabsTrigger value="Data Table">Data Table</VerticalTabsTrigger>
          <VerticalTabsTrigger value="Filter">Filter</VerticalTabsTrigger>
          <VerticalTabsTrigger value="Home">Home</VerticalTabsTrigger>

This should be removed

---

.storybook/main.ts line 10 at r5 (raw file):

    '../src/**/*.mdx',
    '../src/**/*.stories.@(js|jsx|ts|tsx)',
    '../extensions/src/**/*.stories.@(js|jsx|ts|tsx)', // Collect stories from bundled extensions

Can we mark this as a temporary fix? I don't think this is desirable, and we want to revert this once we get Storybook into our extensions

---

extensions/src/platform-get-resources/src/home.component.tsx line 38 at r5 (raw file):

export type HomeDialogProps = {
  localizedStrings?: LanguageStrings;

All these props are optional, which is a little strange to me. I'm sure the component would not work as expected (or at all) if all the props are undefined

---

extensions/src/platform-get-resources/src/home.component.tsx line 40 at r5 (raw file):

  localizedStrings?: LanguageStrings;
  uiLocales?: Intl.LocalesArgument;
  onOpenGetResources?: () => void;

This should be called openGetResources. The current name suggests that this is a function that is called whenever get resources is opened, but that is not the case. It is the actual function that is responsible for opening get resources.

An alternative might be handleOpenGetResources

The same feedback applies to onOpenResourceOrProject and onSendReceiveProject

---

extensions/src/platform-get-resources/src/home.component.tsx line 43 at r5 (raw file):

  onOpenResourceOrProject?: (projectId: string, isEditable: boolean) => void;
  onSendReceiveProject?: (projectId: string) => void;
  showGetResourcesButton?: boolean;

This variable should also be renamed according to our coding standard. Should be something like isShowingGetResourcesButton.
The current name makes it sound like an action, while it represents a state
(Sorry, I think this one is my mistake, lol)

---

extensions/src/platform-get-resources/src/home.component.tsx line 52 at r5 (raw file):

};

export function HomeDialog({

I prefer to just name this component Home, since the component is not really a dialog. Is there any reason you added Dialog to the name?

---

extensions/src/platform-get-resources/src/home.component.tsx line 53 at r5 (raw file):

export function HomeDialog({
  localizedStrings = {},

Along with my remark above (about all the props being optional), it also feels weird to give them these meaningless default values

---

extensions/src/platform-get-resources/src/home.component.tsx line 151 at r5 (raw file):

      setTimeout(() => {
        setProjectResourceFilter('all');
      }, 2000);

This might be more of a UX thing, so ignore it if you don't agree.
I thought it was confusing when I selected a type that I didn't have any resources for, and suddenly the view reset to 'All'. Do we really want to do that, based on a timeout?
Also, in general: I think we should avoid timeouts if we can

---

extensions/src/platform-get-resources/src/home.component.tsx line 259 at r5 (raw file):

  return (
    <div>

This

can be removed

---

extensions/src/platform-get-resources/src/home.component.tsx line 339 at r5 (raw file):

                      <TableRow>
                        <TableHead>
                          <ProjectResourceFilter

Again a UX thing:
I thought it was confusing to find a dropdown on a tableheader. I usually expect to click the header to sort something. Is this the right place for the Filter? Does it make more sense to place it in the top-right part of the webview, outside of the table and near the other filter?

---

extensions/src/platform-get-resources/src/home.component.tsx line 342 at r5 (raw file):

                            onChange={setProjectResourceFilter}
                            variant="ghost"
                            types={Object.values(ProjectTypes)}

This creates a new array every render (even though it's not a big one). It would be better to extract this out into a memoized variable (with useMemo)

---

lib/platform-bible-react/src/preview/pages/components/basics.component.tsx line 58 at r5 (raw file):

          <VerticalTabsTrigger value="Input">Input</VerticalTabsTrigger>
          <VerticalTabsTrigger value="Menubar">Menubar</VerticalTabsTrigger>
          <VerticalTabsTrigger value="ProjResFilter">Project Resource Filter</VerticalTabsTrigger>

This should be removed

---

lib/platform-bible-utils/src/date-time-format-util.ts line 26 at r5 (raw file):

  to = new Date(),
) {
  if (Number.isNaN(since.valueOf())) return '';

Is it okay if this fails silently?

---

extensions/src/platform-get-resources/src/projectResourceFilter.component.tsx line 14 at r5 (raw file):

export type ProjectResourceFilterProps = {
  defaultValue?: ProjectResourceFilterValue;
  onChange: (value: ProjectResourceFilterValue) => void;

Feels odd for this component to have an onChange prop, but not a value prop. Why not get rid of defaultValue and replace it with value? And then hook up the value to the value prop of the Select?

---

extensions/src/platform-get-resources/src/projectResourceFilter.component.tsx line 15 at r5 (raw file):

  defaultValue?: ProjectResourceFilterValue;
  onChange: (value: ProjectResourceFilterValue) => void;
  variant?: 'default' | 'ghost';

This can now be default, ghost or undefined, but we're only setting conditional styling when this is set to ghost. If that is really what we want, this could just be a boolean.
(But maybe we want to use a button instead for the trigger, see my comment below)

---

extensions/src/platform-get-resources/src/projectResourceFilter.component.tsx line 28 at r5 (raw file):

    <Select defaultValue={defaultValue ?? 'all'} onValueChange={onChange}>
      <SelectTrigger
        className={cn('tw-w-16 [&>svg]:tw-min-w-4', {

Does this mimic the look and feel of a ghost button? If so, why not define the asChild prop on the SelectTrigger and just put a Button inside the trigger?

---

extensions/src/platform-get-resources/src/home.web-view.tsx line 46 at r5 (raw file):

import { useCallback, useEffect, useMemo, useRef, useState } from 'react';

const HOME_STRING_KEYS: LocalizeKey[] = [

I think we should move these keys over to the .component.tsx too, and export them from there. We use a similar pattern in various pbr components, such as inventory.component.tsx.
That way we keep the definition and the usage of the keys in the same file

[Sebastian-ubs]

As the discussion around types or interfaces seems not easy to resolve, I have decided to split up this PR and redid the pure splitting of home webview and component in #1728
I will afterwards rebase this PR unto the above mentioned to continue discussing types vs. interfaces

[jolierabideau]

@jolierabideau partially reviewed 28 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 27 unresolved discussions (waiting on @Sebastian-ubs).

---

extensions/src/platform-get-resources/src/project-resource-filter.component.tsx line 33 at r6 (raw file):

}: ProjectResourceFilterProps) {
  const isFiltered = value !== 'all';
  const selectedOption = isFiltered ? options.find((opt) => opt.key === value) : null;

Prefer to use undefined instead of null, usually eslint catches this?

---

c-sharp/Projects/ParatextProjectDataProvider.cs line 985 at r6 (raw file):

            throw new Exception(
                "Cannot set BooksPresent this way. Must add or delete books in the project"
            );

If the PT_IS_RESOURCE is read-only you probably need to add one of these exceptions so people cannot write to it

Code quote:

        if (paratextSettingName == ProjectSettingsNames.PT_BOOKS_PRESENT)
            throw new Exception(
                "Cannot set BooksPresent this way. Must add or delete books in the project"
            );

[Sebastian-ubs]

@Sebastian-ubs made 28 comments and resolved 7 discussions.
Reviewable status: 32 of 34 files reviewed, 21 unresolved discussions (waiting on jolierabideau, lyonsil, and rolfheij-sil).

---

c-sharp/Projects/ParatextProjectDataProvider.cs line 985 at r6 (raw file):

Previously, jolierabideau wrote…

If the PT_IS_RESOURCE is read-only you probably need to add one of these exceptions so people cannot write to it

Done

---

extensions/src/platform-get-resources/src/home.web-view.tsx line 46 at r5 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

I think we should move these keys over to the .component.tsx too, and export them from there. We use a similar pattern in various pbr components, such as inventory.component.tsx.
That way we keep the definition and the usage of the keys in the same file

Done

---

extensions/src/platform-get-resources/src/project-resource-filter.component.tsx line 33 at r6 (raw file):

Previously, jolierabideau wrote…

Prefer to use undefined instead of null, usually eslint catches this?

Done.

---

.storybook/main.ts line 10 at r5 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

Can we mark this as a temporary fix? I don't think this is desirable, and we want to revert this once we get Storybook into our extensions

No longer part of this PR (already on main). I guess I suggest not touching it in here now.

---

extensions/src/platform-get-resources/src/types/project-type.d.ts line 4 at r5 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

All of our exported types should include TSDOC strings.

Done. Change removed

---

extensions/src/platform-get-resources/src/types/project-type.d.ts line 9 at r5 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

We probably need to add comments here on which kind of formatting this is supposed to contain (e.g., BCP-47, ISO-639-1, ISO-639-2, ISO 639-3, etc.).

Done. Change removed

---

extensions/src/platform-get-resources/src/types/project-type.d.ts line 22 at r5 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Why not use Date here instead of string?

Done. Change removed

---

extensions/src/platform-get-resources/src/types/project-type.d.ts line 26 at r5 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I find myself a bit confused on one type of project being project while other types of projects are not project. It feels like we're overloading the term here. Do we mean "project" as in "a distinct collection of data to view and possibly edit?" or "a project as Paratext 9 knows a project"? Or maybe something else?

Perhaps the key should be renamed to paratext9 (or something very similar) so that "project" means "a distinct collection of data". If we don't want to do that, then I would suggest need a new meaning for "a distinct collection of data" regardless of whether it's an editable translation, a resource, a dictionary, media, etc.

Done. Using "Paratext Project" and "Resource" now.

---

extensions/src/platform-get-resources/src/types/project-type.d.ts line 28 at r5 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

At first glance this type is a little confusing based on its name alone. I think I realize what this is intended to represent now after some thought, but adding TSDOCs and renaming to something like ProjectTypeAction would help future reviewers.

Done. Change removed

---

extensions/src/platform-get-resources/src/types/project-type.d.ts line 31 at r5 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I don't understand the purpose of condition, and examples in project-types.ts all return true. If the idea is whether type action can apply to a particular project, I would expect there to be an input argument to represent the project. And really I would expect the same argument type(s) to be passed to action and default.

Done. Change removed

---

extensions/src/platform-get-resources/src/types/project-type.d.ts line 36 at r5 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

This implies that the values are localized, but we are hard-coding English values in our types. It would probably be better to specify the localization key in this type and let the UI look up the actual localized name.

Done. Change removed

---

lib/platform-bible-react/src/preview/pages/components/advanced.component.tsx line 81 at r5 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

This should be removed

Done. Change removed

---

lib/platform-bible-react/src/preview/pages/components/basics.component.tsx line 58 at r5 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

This should be removed

Done. Change removed

---

lib/platform-bible-utils/src/date-time-format-util.ts line 26 at r5 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

Is it okay if this fails silently?

Done. Change removed

---

extensions/src/platform-get-resources/src/home.component.tsx line 38 at r5 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

All these props are optional, which is a little strange to me. I'm sure the component would not work as expected (or at all) if all the props are undefined

There is a story NoProjectsNoHeader using it without parameters, so I guess this is fine.

---

extensions/src/platform-get-resources/src/home.component.tsx line 40 at r5 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

This should be called openGetResources. The current name suggests that this is a function that is called whenever get resources is opened, but that is not the case. It is the actual function that is responsible for opening get resources.

An alternative might be handleOpenGetResources

The same feedback applies to onOpenResourceOrProject and onSendReceiveProject

No longer part of this PR, should we change it here?

---

extensions/src/platform-get-resources/src/home.component.tsx line 43 at r5 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

This variable should also be renamed according to our coding standard. Should be something like isShowingGetResourcesButton.
The current name makes it sound like an action, while it represents a state
(Sorry, I think this one is my mistake, lol)

No longer part of this PR, should we change it here?

---

extensions/src/platform-get-resources/src/home.component.tsx line 52 at r5 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

I prefer to just name this component Home, since the component is not really a dialog. Is there any reason you added Dialog to the name?

Done.

---

extensions/src/platform-get-resources/src/home.component.tsx line 53 at r5 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

Along with my remark above (about all the props being optional), it also feels weird to give them these meaningless default values

There is a story NoProjectsNoHeader using it without parameters, so I guess this is fine.

---

extensions/src/platform-get-resources/src/home.component.tsx line 151 at r5 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

This might be more of a UX thing, so ignore it if you don't agree.
I thought it was confusing when I selected a type that I didn't have any resources for, and suddenly the view reset to 'All'. Do we really want to do that, based on a timeout?
Also, in general: I think we should avoid timeouts if we can

Done. Was removed

---

extensions/src/platform-get-resources/src/home.component.tsx line 259 at r5 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

This

can be removed

? what can be removed?

---

extensions/src/platform-get-resources/src/home.component.tsx line 339 at r5 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

Again a UX thing:
I thought it was confusing to find a dropdown on a tableheader. I usually expect to click the header to sort something. Is this the right place for the Filter? Does it make more sense to place it in the top-right part of the webview, outside of the table and near the other filter?

Done. Is now done differently.

---

extensions/src/platform-get-resources/src/home.component.tsx line 342 at r5 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

This creates a new array every render (even though it's not a big one). It would be better to extract this out into a memoized variable (with useMemo)

Not (no longer?) part of this PR. Should we change it here?

---

extensions/src/platform-get-resources/src/projectResourceFilter.component.tsx line 14 at r5 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

Feels odd for this component to have an onChange prop, but not a value prop. Why not get rid of defaultValue and replace it with value? And then hook up the value to the value prop of the Select?

done (the new filename is project-resource-filter.component.tsx)

---

extensions/src/platform-get-resources/src/projectResourceFilter.component.tsx line 15 at r5 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

This can now be default, ghost or undefined, but we're only setting conditional styling when this is set to ghost. If that is really what we want, this could just be a boolean.
(But maybe we want to use a button instead for the trigger, see my comment below)

Done. Variant is no longer exported, see below.
(the new filename is project-resource-filter.component.tsx)

---

extensions/src/platform-get-resources/src/projectResourceFilter.component.tsx line 28 at r5 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

Does this mimic the look and feel of a ghost button? If so, why not define the asChild prop on the SelectTrigger and just put a Button inside the trigger?

This was now changed to use a toggle group button with variant "outline", but does not expose the variant as a prop.
(the new filename is project-resource-filter.component.tsx)

---

extensions/src/platform-get-resources/src/types/project-types.ts line 7 at r5 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Just noting my object here again to using project to mean both a generic kind of thing we open and a specific kind of scripture-related thing with a set of files that PT9 understands and we inherit through ParatextData.dll.

Done. Change removed

[jolierabideau]

Thanks Sebastian!

@jolierabideau reviewed 2 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on lyonsil and rolfheij-sil).

[Sebastian-ubs]

Thank you @jolierabideau.
I just found there was a problem with filtering in New Tab.
Meanwhile I also introduced a new icon for read-only projects and improved the summary.

[tjcouch-sil]

Thanks for all the hard work on simplifying this massively. I'm glad this is finally moving again! Though one discussion still isn't finished that is part of this work (I added a comment here about it).

@tjcouch-sil partially reviewed 35 files and all commit messages, and made 5 comments.
Reviewable status: all files reviewed, 23 unresolved discussions (waiting on lyonsil, rolfheij-sil, and Sebastian-ubs).

---

extensions/src/platform-get-resources/contributions/localizedStrings.json line 25 at r9 (raw file):

      "%resources_installed%": "Installed",
      "%resources_items%": "items",
      "%resources_itemsFiltered%": "{0} out of {1} items",

For all of these strings you added that have {something} in them, please replace the number with a word or lowerCamelCase words that describe what is supposed to go there. See other format strings for examples of what I'm talking about. This makes it easier to read and for translators to translate.

---

src/declarations/papi-shared-types.ts line 263 at r9 (raw file):

     * the IsResourceProject property from Paratext.
     */
    'platform.isResource': boolean;

Did we ever finish the conversation regarding how to handle resources? It doesn't seem to me based on any of the discussions I can find.

Essentially, my concern is this:

"Resource" still seems to me to be too imprecise a term and a distinction from "project" for me to want to use it. In our API, we are using different terms than we do in the UI. A Paratext "project" (as it is called in the UI) is a specific kind of API "project" that has Scripture and such things. Up to this point, I haven't received a precise explanation of what makes a Paratext "resource" distinct from a Paratext "project", and that makes it basically impossible for me to know how best to represent that difference on the API. Once I know what the real difference is, I can give better recommendation as to what to do about the difference.

Maybe we just need to call it something more precise to what the term actually means in effect on the API. For example, if it means it's published, maybe you can call it platform.isPublished. If it means it's completely read-only in all ways, maybe you can call it platform.isFullyReadOnly (though I'm not really in love with this name or this approach).

A boolean setting may very well be a clear and simple way to represent it. Or it could be that we deprecate platform.isEditable and make a new three-state setting. Or we deprecate platform.isEditable and make a new setting that is an array of things that are readonly (and make a way to specify "everything is read-only"). Or something else.

I could definitely see us for now just going for a new boolean. No matter what we do, though, I need to know the functional difference between Paratext "project" and "resource" to know what to do.

If we do go for the two-boolean approach, we need to make sure to document very clearly the definitions of "editable" vs "resource" and the implications of the various states:

• isEditable: true; isResource: false
• isEditable: true; isResource: true
• isEditable: false; isResource: false
• isEditable: false; isResource: true

---

If you'd prefer to defer this discussion until later, you can simply revert to using platform.isEditable as we have elsewhere so far.

---

(The following links are for reference; no need to look unless you want to get more context)

These two messages from our existing discussion seem like they capture my unanswered concerns well:

• https://discord.com/channels/1064938364597436416/1370390106833424474/1371848936045678672 (the most important problem)
• https://discord.com/channels/1064938364597436416/1370390106833424474/1392499704255746170

---

extensions/src/platform-get-resources/src/home.component.tsx line 479 at r9 (raw file):

                        ? noSearchResultsText
                        : projectResourceFilter !== 'all'
                          ? noItemsFoundText.replace(

It's best to avoid nested ternary operators because of the mental complexity of reading them. You can put this logic above the JSX and just set a variable to whatever you want to show up in here (e.g. use if statements above).

There is a lint error reporting this problem as well, but I'm just giving some context as to why this is a rule and how to adjust.

---

extensions/src/platform-get-resources/src/home.component.tsx line 479 at r9 (raw file):

                        ? noSearchResultsText
                        : projectResourceFilter !== 'all'
                          ? noItemsFoundText.replace(

Is there a particular reason you didn't use formatReplacementString here? And in the .replaces added below

[Sebastian-ubs]

@tjcouch-sil ready again for review