Update Get Resources web-view UI to Phase 1 UX design

[rolfheij-sil]

Design spec found here

---

This change is 

[lyonsil]

Looks pretty good overall. I have a couple of blocking considerations and a few other questions.

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

---

extensions/src/platform-home/src/home.web-view.tsx line 0 at r1 (raw file):
This looks identical to get-resources.web-view.tsx. Even if there are just one or two lines of difference, why aren't we refactoring to provide a single component that both of these web views use? I guess I'm wondering, like in my previous comment, why we are even creating separate extensions if we're just duplicating code.

---

extensions/src/platform-home/contributions/localizedStrings.json line 0 at r1 (raw file):
It doesn't make a lot of sense to me that we're creating multiple extensions with identical localizations that are all being used to implement the same UI. Since these are all bundled extensions anyway, why did we split these up into separate extensions? If we really do want them to be separate extensions, then we really ought to just enforce a dependency between the extensions and make sure we load them in the right order. Then we don't have to worry about whether the localized strings we need exist.

What might even be simpler than enforcing a dependency between extensions would be to put these in core's localized strings file.

Bottom line, defining the same localized strings in multiple extensions is a recipe for problems later. Some strings will inevitably be updated in one but not the other, and then the indeterminate loading order of the extensions will result in inconsistent localized strings being shown.

---

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

}: FilterableResourceListProps) {
  const actionText: string = localizedStrings['%resources_action%'];
  const anyText: string = localizedStrings['%resources_any%'];

Perhaps the reason we're duplicating the definition of these localized strings is that anything that uses these components needs the strings to be defined. Is that right? If so, let's move these strings into the base platform's localized strings. Otherwise anytime a new extension is added that uses this component we'll have to create yet another copy.

---

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

              />

              <Filter

Just a question about this - I would have guessed that a filter for selecting languages would use the new language picker control, not a generic filter. The language picker should handle things like providing the correct label to display for a resource's language matching the UI language. (e.g., if the UI language is Spanish, I think it would say "Inglés" for English resources) We don't really want to be defining language names ourselves if the language picker is already going to handle that using SLDR or something like that.

Was this part of phase 1? Maybe this comes in a later phase. Just checking.

---

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.component.tsx line 56 at r1 (raw file):

  const getPlaceholderText = () => {
    if (selected.length === 1)

Just curious - why was this removed?

[Sebastian-ubs]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @lyonsil and @rolfheij-sil)

---

extensions/src/platform-home/src/home.web-view.tsx line at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

This looks identical to get-resources.web-view.tsx. Even if there are just one or two lines of difference, why aren't we refactoring to provide a single component that both of these web views use? I guess I'm wondering, like in my previous comment, why we are even creating separate extensions if we're just duplicating code.

I think putting it into platform-bible-react makes sense for 2 reasons:

1. the close proximity to Home. Not sure however if the remaining differences are worth re-using the whole ui or only parts.
2. having whole uis in pbr allows for quicker ui changes and reuse of parts later, as well as possibility to immediately test it with the color theme and alignment

---

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

Previously, lyonsil (Matt Lyons) wrote…

Just a question about this - I would have guessed that a filter for selecting languages would use the new language picker control, not a generic filter. The language picker should handle things like providing the correct label to display for a resource's language matching the UI language. (e.g., if the UI language is Spanish, I think it would say "Inglés" for English resources) We don't really want to be defining language names ourselves if the language picker is already going to handle that using SLDR or something like that.

Was this part of phase 1? Maybe this comes in a later phase. Just checking.

No these are different use cases. Probably the way to get the language names can later be shared, but the controls themselves are different:

• ui language picker is a single select component without filtering
• resource language filter is a multi select component with filtering and sorting the selected to the top

---

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.component.tsx line 56 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Just curious - why was this removed?

This was removed as per the new design

---

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

                      placeholder={filterInputText}
                    />
                    <Search className="tw-absolute tw-right-3 tw-top-1/2 tw-h-4 tw-w-4 tw--translate-y-1/2 tw-transform tw-text-muted-foreground" />

The search icon is on the wrong side (and will not change in rtl, but this is discussed elsewhere). Would using the searchBar component work instead?
This is not blocking - we could change this later.

[Sebastian-ubs]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @lyonsil and @rolfheij-sil)

---

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

Previously, Sebastian-ubs wrote…

No these are different use cases. Probably the way to get the language names can later be shared, but the controls themselves are different:

• ui language picker is a single select component without filtering
• resource language filter is a multi select component with filtering and sorting the selected to the top

After testing:

1. Good that you added a realistic amount of languages already to the preview app. I recognized, that the language filter popover needs >1 second to open and also a little more than the usual time to close. I guess this is due to the large number of languages (dom nodes) in the popover. We may want to file a separate issue to look into improving this (e.g. not adding+removing it from the dom, but only hiding it).

2. The starred items seem not to match with the initial selection right now, this should be fixed.
   

[Sebastian-ubs]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @lyonsil and @rolfheij-sil)

---

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

Previously, Sebastian-ubs wrote…

After testing:

1. Good that you added a realistic amount of languages already to the preview app. I recognized, that the language filter popover needs >1 second to open and also a little more than the usual time to close. I guess this is due to the large number of languages (dom nodes) in the popover. We may want to file a separate issue to look into improving this (e.g. not adding+removing it from the dom, but only hiding it).

2. The starred items seem not to match with the initial selection right now, this should be fixed.
   

Found additional bugs:

3. Selected items are not consistently sorted to the top. Also the rest is not alphabetically sorted.
   Sorting should be: starred items A-z (independent of their selection), selected (non-starred) items A-z, other (non-starred, non-selected) A-z
   English language names are fine for now. This is what we get from DBL and this is what PT9 shows. We may improve this in the future.

4. Unexpectedly, filtering the language seems to include text this is not shown or does a non-strict search on the language name and acronym. Also compared to PT9, this is showing too much.

[rolfheij-sil]

Reviewable status: 13 of 20 files reviewed, 5 unresolved discussions (waiting on @lyonsil and @Sebastian-ubs)

---

extensions/src/platform-home/contributions/localizedStrings.json line at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

It doesn't make a lot of sense to me that we're creating multiple extensions with identical localizations that are all being used to implement the same UI. Since these are all bundled extensions anyway, why did we split these up into separate extensions? If we really do want them to be separate extensions, then we really ought to just enforce a dependency between the extensions and make sure we load them in the right order. Then we don't have to worry about whether the localized strings we need exist.

What might even be simpler than enforcing a dependency between extensions would be to put these in core's localized strings file.

Bottom line, defining the same localized strings in multiple extensions is a recipe for problems later. Some strings will inevitably be updated in one but not the other, and then the indeterminate loading order of the extensions will result in inconsistent localized strings being shown.

Yeah, good catch. Initally Get Resources was in platform-bible-internal-extensions and Home was supposed to be bundled, so that's why they're separate extensions. Now that they're both bundled we can probably merge them into a single extension. I'll probably do that in a different PR though, is that okay?

---

extensions/src/platform-home/src/home.web-view.tsx line at r1 (raw file):

Previously, Sebastian-ubs wrote…

I think putting it into platform-bible-react makes sense for 2 reasons:

1. the close proximity to Home. Not sure however if the remaining differences are worth re-using the whole ui or only parts.
2. having whole uis in pbr allows for quicker ui changes and reuse of parts later, as well as possibility to immediately test it with the color theme and alignment

@lyonsil Yes, this branch does not have a proper implementation of the Home design as proposed by UX yet. I just plugged in a copy of the Get Resources web view as a placeholder for now, since their designs are very similar. I'm working on implementing the Home design on a separate branch

@Sebastian-ubs Thinking about it, if Get Resources and Home do not turn out as similar as you were hoping, we can always split up FilterableResourceList in to smaller components. For example, yesterday Alex told me that the Home design will not have the Type and Language filter.

---

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

Previously, lyonsil (Matt Lyons) wrote…

Perhaps the reason we're duplicating the definition of these localized strings is that anything that uses these components needs the strings to be defined. Is that right? If so, let's move these strings into the base platform's localized strings. Otherwise anytime a new extension is added that uses this component we'll have to create yet another copy.

Good catch. I guess there's two options: Move the localized strings to base, or merge the extensions. I'm not sure what the best approach is, especially because the Home design is still evolving, so I don't know how similar/different Get Resources and Home will end up in the long run

---

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

Previously, Sebastian-ubs wrote…

The search icon is on the wrong side (and will not change in rtl, but this is discussed elsewhere). Would using the searchBar component work instead?
This is not blocking - we could change this later.

Yep, using the search bar is a good idea. I think I got the current search input from the Book Chapter selector. I didn't know about the searchbar 😅

---

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

Previously, Sebastian-ubs wrote…

Found additional bugs:

3. Selected items are not consistently sorted to the top. Also the rest is not alphabetically sorted.
   Sorting should be: starred items A-z (independent of their selection), selected (non-starred) items A-z, other (non-starred, non-selected) A-z
   English language names are fine for now. This is what we get from DBL and this is what PT9 shows. We may improve this in the future.

4. Unexpectedly, filtering the language seems to include text this is not shown or does a non-strict search on the language name and acronym. Also compared to PT9, this is showing too much.

1. Good idea. I think there's multiple places in our code that could be optimized for performance. There might already be an issue for that somewhere, and if not we should open one.
2. Yep, good catch. Will investigate.
3. I believe the sorting happens in the way you describe. The screenshot you added also shows this sorting behavior, right?
4. This behavior is defined by a search algorithm somewhere very deep down in the code. TheMultiSelectComboBoxuses the shadcn Command, which uses this component, which has this sorting logic. I suggest we do not change it.

---

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.component.tsx line 56 at r1 (raw file):

Previously, Sebastian-ubs wrote…

This was removed as per the new design

Initially the multi-selects related to the filters were designed to always have at least one option selected. However, this meant that showing resources in all languages would require the user to manually select all languages in the filter. So the component was redesigned so that selecting 0 items is interpreted as 'any items', effectively showing all items

[Sebastian-ubs]

Reviewable status: 13 of 20 files reviewed, 5 unresolved discussions (waiting on @lyonsil and @rolfheij-sil)

---

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

Previously, rolfheij-sil (Rolf Heij) wrote…

Yep, using the search bar is a good idea. I think I got the current search input from the Book Chapter selector. I didn't know about the searchbar 😅

just be aware, that the search bar is currently a bit broken ^^
(but I guess we want to fix that soon anyhow)
#1405

---

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

Previously, rolfheij-sil (Rolf Heij) wrote…

1. Good idea. I think there's multiple places in our code that could be optimized for performance. There might already be an issue for that somewhere, and if not we should open one.
2. Yep, good catch. Will investigate.
3. I believe the sorting happens in the way you describe. The screenshot you added also shows this sorting behavior, right?
4. This behavior is defined by a search algorithm somewhere very deep down in the code. TheMultiSelectComboBoxuses the shadcn Command, which uses this component, which has this sorting logic. I suggest we do not change it.

1. I will add one to epic 117
3. Sorry, yes the screenshot kind of shows sorting for starred and selected items correctly. I however also had a non-starred item appear in the middle of stars at some point. Also the list of unselected languages starts with a language with N, but should be with a language with A
4. Sad to hear this is deep inside the provided code. From a user perspective I think we want to change this. Is it filtering based on some other properties of the object that you pass in, or only based on the strings that show up in the combobox popover but "fuzzy"?

[Sebastian-ubs]

Reviewable status: 13 of 20 files reviewed, 5 unresolved discussions (waiting on @lyonsil and @rolfheij-sil)

---

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

Previously, Sebastian-ubs wrote…

1. I will add one to epic 117
3. Sorry, yes the screenshot kind of shows sorting for starred and selected items correctly. I however also had a non-starred item appear in the middle of stars at some point. Also the list of unselected languages starts with a language with N, but should be with a language with A
4. Sad to hear this is deep inside the provided code. From a user perspective I think we want to change this. Is it filtering based on some other properties of the object that you pass in, or only based on the strings that show up in the combobox popover but "fuzzy"?

thinking about 3, maybe we need a divider between any of these, but this seems to be minor improvements that can come later. Sorting alphabetically however is needed to not get lost.

[lyonsil]

Reviewed 7 of 7 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @rolfheij-sil and @Sebastian-ubs)

---

extensions/src/platform-home/contributions/localizedStrings.json line at r1 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

Yeah, good catch. Initally Get Resources was in platform-bible-internal-extensions and Home was supposed to be bundled, so that's why they're separate extensions. Now that they're both bundled we can probably merge them into a single extension. I'll probably do that in a different PR though, is that okay?

Since we don't have a way for libraries to contribute localization files, then I think any localization strings used by the react lib and utilities lib should be included in the platform. Please move them out of the extensions and into the main localization file for the platform.

---

extensions/src/platform-home/src/home.web-view.tsx line at r1 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

@lyonsil Yes, this branch does not have a proper implementation of the Home design as proposed by UX yet. I just plugged in a copy of the Get Resources web view as a placeholder for now, since their designs are very similar. I'm working on implementing the Home design on a separate branch

@Sebastian-ubs Thinking about it, if Get Resources and Home do not turn out as similar as you were hoping, we can always split up FilterableResourceList in to smaller components. For example, yesterday Alex told me that the Home design will not have the Type and Language filter.

Ok - If we think these will end up diverging I'm OK with them being duplicates for now. It would be good to combine them into a single control, though, if they end up being identical or nearly identical.

---

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

Previously, rolfheij-sil (Rolf Heij) wrote…

Good catch. I guess there's two options: Move the localized strings to base, or merge the extensions. I'm not sure what the best approach is, especially because the Home design is still evolving, so I don't know how similar/different Get Resources and Home will end up in the long run

I'll resolve this comment and leave the other one open related to this topic. Since we don't have a way to add localization strings from libraries, we can move these to the main platform's localization file.

---

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.component.tsx line 56 at r1 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

Initially the multi-selects related to the filters were designed to always have at least one option selected. However, this meant that showing resources in all languages would require the user to manually select all languages in the filter. So the component was redesigned so that selecting 0 items is interpreted as 'any items', effectively showing all items

Thanks for the explanation!

[lyonsil]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @rolfheij-sil and @Sebastian-ubs)

---

extensions/src/platform-home/src/home.web-view.tsx line at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Ok - If we think these will end up diverging I'm OK with them being duplicates for now. It would be good to combine them into a single control, though, if they end up being identical or nearly identical.

I should have said "a single component" in my previous comment.

[rolfheij-sil]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Sebastian-ubs)

---

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

Previously, Sebastian-ubs wrote…

thinking about 3, maybe we need a divider between any of these, but this seems to be minor improvements that can come later. Sorting alphabetically however is needed to not get lost.

3. I'm trying to replicate the behavior you mention, but I can't. I don't ever see unstarred things in between the starred items, and everything sorts correctly A-z. If you can still replicate this, let me know and we can hop on a call and I can take a look with you.
4. I don't know if we can control it through props. Maybe we could add a separate issue for this, since it's more about customizing the Command component than having anything to do with Get Resources

[rolfheij-sil]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Sebastian-ubs)

---

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

Previously, rolfheij-sil (Rolf Heij) wrote…

3. I'm trying to replicate the behavior you mention, but I can't. I don't ever see unstarred things in between the starred items, and everything sorts correctly A-z. If you can still replicate this, let me know and we can hop on a call and I can take a look with you.
4. I don't know if we can control it through props. Maybe we could add a separate issue for this, since it's more about customizing the Command component than having anything to do with Get Resources

Note: Issues 3 and 4 seem to be related.
@Sebastian-ubs and I have found that the sorting logic when performing a search on the dropdown is controlled by a custom search algorithm in one of the underlying components on the Multi Select. This leads to very unpredictable sorting results. A separate issue will be opened to investigate this.

[Sebastian-ubs]

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

---

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

Previously, rolfheij-sil (Rolf Heij) wrote…

Note: Issues 3 and 4 seem to be related.
@Sebastian-ubs and I have found that the sorting logic when performing a search on the dropdown is controlled by a custom search algorithm in one of the underlying components on the Multi Select. This leads to very unpredictable sorting results. A separate issue will be opened to investigate this.

I've created #1410 and #1411. As the bug about starred item selection was fixed, I am resolving this.

[Sebastian-ubs]

please still fix the issue of the wrong field used for filtering the type popover

[Sebastian-ubs]

Reviewable status: 9 of 21 files reviewed, 1 unresolved discussion (waiting on @lyonsil and @rolfheij-sil)

[rolfheij-sil]

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

---

extensions/src/platform-home/contributions/localizedStrings.json line at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Since we don't have a way for libraries to contribute localization files, then I think any localization strings used by the react lib and utilities lib should be included in the platform. Please move them out of the extensions and into the main localization file for the platform.

Done.