feat: simplify library home [FC-0062] #1443

DanielVZ96 · 2024-10-27T14:28:10Z

Description

Simplifies the library UI/UX and code by using the same grid for the home, components and collections tab, also unifying the query used for collections and the react component used to display library content grids.

Supporting Information

Implements: #1230
Private Ref: FAL-3918

Testing Instructions

Create a new Library
Inside the new library create a new component and collection.
- Verify they both display in the "All Content" tab.
- Verify that only the component appears in the "Components" tab
- Verify that only the collection appears in the "Collections" tab
- Verify that you can filter by the component type correctly
Now add dozens of components by copy and pasting them
- Verify that after a certain number if you refresh the page they are loaded in an infinite scroll
- Verify that you can sort them and filter them correctly.
Open the collection and verify you can both add new components and select components from the library by clicking "New" and "Existing Library Content"
Click on the search modal on top and then try to find XBlocks that are used in courses (we have to test this because the search results typing had to be changed a bit, but there should be no issue here).
Now navigate to http://apps.local.openedx.io:2001/authoring/library/component-picker and verify components and collections render correctly.

openedx-webhooks · 2024-10-27T14:28:14Z

Thanks for the pull request, @DanielVZ96!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/2u-tnl. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

src/search-manager/SearchManager.ts

src/search-manager/data/api.ts

feat: simplify library home

DanielVZ96 · 2024-11-03T05:23:52Z

src/library-authoring/LibraryContent.tsx

@@ -0,0 +1,86 @@
+import { useEffect } from 'react';


Replaced LibraryHome, LibraryCollections and Library Components with this single file that does the same based on the content prop.

codecov · 2024-11-03T05:31:41Z

Codecov Report

Attention: Patch coverage is 97.91667% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.23%. Comparing base (949e4ac) to head (9165f01).
Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
src/search-manager/data/api.ts	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1443      +/-   ##
==========================================
- Coverage   93.27%   93.23%   -0.04%     
==========================================
  Files        1052     1048       -4     
  Lines       20462    20401      -61     
  Branches     4298     4286      -12     
==========================================
- Hits        19086    19021      -65     
- Misses       1316     1320       +4     
  Partials       60       60

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

DanielVZ96 · 2024-11-03T06:00:12Z

Coverage went down due to removed previously covered code.

bradenmacdonald

Nice, this looks great! I only tested it briefly but it seems to be working well. Will let @rpenido test it fully.

bradenmacdonald · 2024-11-04T22:18:53Z

src/search-modal/SearchUI.test.tsx

+        // ['', 'type = "course_block"', 'context_key = "course-v1:org+test+123"']
+        return (requestedFilter?.length === 3);


Is the first empty string '' necessary?

Not sure why it is there. It was like that before but I can check.

The first '' is probably from the empty search query.

It's not a big deal. I think it's an artifact of this:

frontend-app-authoring/src/search-manager/data/api.ts

Lines 243 to 247 in fc94667

// To filter normal block types and problem types as 'OR' query

const typeFilters = [[

...blockTypesFilterFormatted,

...problemTypesFilterFormatted,

].flat()];

If there are no filters set, this still adds an empty array [] to the filters list. It doesn't cause any problems.

bradenmacdonald · 2024-11-04T22:22:28Z

src/library-authoring/LibraryContent.tsx

+ */
+
+type LibraryContentProps = {
+  content: string;


Suggested change

content: string;

content: TabList;

Can you give this a more specific type than string? Maybe use the same enum we already have defined, but rename it from TabList to ContentType ?

Or even better, get rid of the content prop and just add an emptyState: React.ReactNode component. Then, below, you can do something like this:

if (totalHits === 0) { if (props.emptyState) { return <>{props.emptyState}</>; } return isFiltered ? <NoSearchResults /> : <NoComponents handleBtnClick={openAddContentSidebar} />; }

bradenmacdonald · 2024-11-04T22:43:51Z

src/search-manager/data/api.ts

@@ -558,7 +526,7 @@ export async function fetchTagsThatMatchKeyword({
 * Fetch single document by its id
 */
 /* istanbul ignore next */
-export async function fetchDocumentById({ client, indexName, id } : {
+export async function fetchDocumentById({ client, indexName, id }: {


Actually I think this fetchDocumentById function as well as useGetSingleDocument are unused. You can just remove them while you're at it if you want.

rpenido

Great work here, @DanielVZ96!
This is a really big change, and you handled it graciously!

There is a small bug with the Manage Collections feature. It is an easy fix.
I left some suggestions on the review, but feel free to skip the ones you don't think are relevant.

Also, if you have more time, could you take a look at the LibraryAuthoringPage and the LibraryCollectionPage components?
They are a level above the new LibraryContent component and share a lot of code.
We can also merge them, so if we implement a new filter or add the Units tab, we could do it in one place. If you think it is a complicated merge, we can do this on another task.

rpenido · 2024-11-05T14:28:11Z

src/library-authoring/component-info/ManageCollections.tsx

@@ -116,7 +116,7 @@ const AddToCollectionsDrawer = ({ usageKey, collections, onClose }: CollectionsD
        components: { limit: 0 },


The Manage Collections feature is not showing any hits.

Removing the line below fix it:

Suggested change

components: { limit: 0 },

rpenido · 2024-11-05T14:29:20Z

src/search-manager/data/api.ts

-  if (overrideQueries?.collections) {
-    newQueries[2] = { ...overrideQueries.collections, indexUid: queries[2].indexUid };
-  }


We should also remove the collections property from the OverrideQueries type.

Edit: Maybe rename the components as the query brings components and collections.

rpenido · 2024-11-05T14:37:55Z

src/search-modal/SearchResults.tsx

@@ -28,7 +28,7 @@ const SearchResults: React.FC<Record<never, never>> = () => {

  return (
    <>
-      {hits.map((hit) => <SearchResult key={hit.id} hit={hit} />)}
+      {hits.filter(hit => hit.type !== 'collection').map((hit) => <SearchResult key={hit.id} hit={hit as ContentHit} />)}


If we make the types more specific (bellow diff) we can do better type inference

diff --git a/src/search-manager/data/api.ts b/src/search-manager/data/api.ts index 8d1679ec..7f8c8a21 100644 --- a/src/search-manager/data/api.ts +++ b/src/search-manager/data/api.ts @@ -119,6 +119,7 @@ interface BaseContentHit { * Defined in edx-platform/openedx/core/djangoapps/content/search/documents.py */ export interface ContentHit extends BaseContentHit { + type: 'course_block' | 'library_block'; /** The block_type part of the usage key. What type of XBlock this is. */ blockType: string; /** @@ -149,6 +150,7 @@ export interface ContentPublishedData { * Defined in edx-platform/openedx/core/djangoapps/content/search/documents.py */ export interface CollectionHit extends BaseContentHit { + type: 'collection'; description: string; numChildren?: number; }

Suggested change

{hits.filter(hit => hit.type !== 'collection').map((hit) => <SearchResult key={hit.id} hit={hit as ContentHit} />)}

{hits.filter((hit): hit is ContentHit => hit.type !== 'collection').map(

(hit) => <SearchResult key={hit.id} hit={hit} />,

)}

rpenido · 2024-11-05T14:40:16Z

src/library-authoring/LibraryContent.tsx

+        contentHit.type === 'collection' ? (
+          <CollectionCard
+            key={contentHit.id}
+            collectionHit={contentHit as CollectionHit}
+          />
+        ) : (
+          <ComponentCard
+            key={contentHit.id}
+            contentHit={contentHit as ContentHit}
+          />
+        )


Also, with the change above, the contentHit.type === 'collection' becomes a type guard

Suggested change

contentHit.type === 'collection' ? (

<CollectionCard

key={contentHit.id}

collectionHit={contentHit as CollectionHit}

/>

) : (

<ComponentCard

key={contentHit.id}

contentHit={contentHit as ContentHit}

/>

)

contentHit.type === 'collection' ? (

<CollectionCard

key={contentHit.id}

collectionHit={contentHit}

/>

) : (

<ComponentCard

key={contentHit.id}

contentHit={contentHit}

/>

)

rpenido · 2024-11-05T14:51:24Z

src/library-authoring/components/index.ts

Not related to this change, but there are a lot of places where we are breaking the feature boundaries and using:

import { SomeComponent } from './components/SomeComponent' import { AnotherComponent } from './components/AnotherComponent'

Instead of

import { SomeComponent, AnotherComponent } from './components'

Could you check if this is easy to fix?
If not, just remove this index.ts and directly import the ComponentMenu when used.

Is there a policy on this? Personally I always use the first approach because it's less likely to cause circular dependencies and it's slightly faster during code analysis and builds.

We have this: https://github.com/openedx/frontend-template-application/blob/master/docs/decisions/0002-feature-based-application-organization.rst
But I'm not sure that our "features" are decoupled enough to follow it.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 27, 2024

DanielVZ96 force-pushed the dvz/simplify-home branch 3 times, most recently from 3ee85ce to 2cef883 Compare October 27, 2024 19:58

bradenmacdonald changed the title ~~feat: simplify library home~~ feat: simplify library home [FC-0062] Oct 29, 2024

bradenmacdonald added the FC Relates to an Axim Funded Contribution project label Oct 29, 2024

bradenmacdonald reviewed Oct 29, 2024

View reviewed changes

src/search-manager/SearchManager.ts Outdated Show resolved Hide resolved

bradenmacdonald reviewed Oct 29, 2024

View reviewed changes

src/search-manager/data/api.ts Outdated Show resolved Hide resolved

DanielVZ96 added 2 commits November 3, 2024 02:21

feat: simplify home interface

c7ca212

feat: simplify library home

feat: use single query for all content

59815e5

DanielVZ96 force-pushed the dvz/simplify-home branch from 93171c3 to 9966398 Compare November 3, 2024 05:21

DanielVZ96 commented Nov 3, 2024

View reviewed changes

DanielVZ96 force-pushed the dvz/simplify-home branch from 9966398 to 1e7337a Compare November 3, 2024 05:53

DanielVZ96 marked this pull request as ready for review November 3, 2024 05:59

DanielVZ96 requested a review from a team as a code owner November 3, 2024 05:59

DanielVZ96 requested a review from bradenmacdonald November 3, 2024 06:00

feat: unify library component and collections listing

d9be05d

DanielVZ96 force-pushed the dvz/simplify-home branch from 1e7337a to d9be05d Compare November 3, 2024 17:24

bradenmacdonald reviewed Nov 5, 2024

View reviewed changes

fix: use ContentType enum

8a9fb8b

rpenido suggested changes Nov 5, 2024

View reviewed changes

DanielVZ96 added 2 commits November 5, 2024 22:09

fixup! fix: use ContentType enum

c0c8a9a

fixup! fixup! fix: use ContentType enum

9165f01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: simplify library home [FC-0062] #1443

feat: simplify library home [FC-0062] #1443

DanielVZ96 commented Oct 27, 2024 •

edited

Loading

openedx-webhooks commented Oct 27, 2024 •

edited

Loading

DanielVZ96 Nov 3, 2024

codecov bot commented Nov 3, 2024 •

edited

Loading

DanielVZ96 commented Nov 3, 2024 •

edited

Loading

bradenmacdonald left a comment

bradenmacdonald Nov 4, 2024

DanielVZ96 Nov 5, 2024

rpenido Nov 5, 2024 •

edited

Loading

bradenmacdonald Nov 5, 2024

bradenmacdonald Nov 4, 2024

bradenmacdonald Nov 4, 2024

rpenido left a comment

rpenido Nov 5, 2024

rpenido Nov 5, 2024 •

edited

Loading

rpenido Nov 5, 2024

bradenmacdonald Nov 5, 2024

rpenido Nov 5, 2024

rpenido Nov 5, 2024

bradenmacdonald Nov 5, 2024

rpenido Nov 5, 2024

		// ['', 'type = "course_block"', 'context_key = "course-v1:org+test+123"']
		return (requestedFilter?.length === 3);

	// To filter normal block types and problem types as 'OR' query
	const typeFilters = [[
	...blockTypesFilterFormatted,
	...problemTypesFilterFormatted,
	].flat()];

		@@ -116,7 +116,7 @@ const AddToCollectionsDrawer = ({ usageKey, collections, onClose }: CollectionsD
		components: { limit: 0 },

-      {hits.filter(hit => hit.type !== 'collection').map((hit) => <SearchResult key={hit.id} hit={hit as ContentHit} />)}
+      {hits.filter((hit): hit is ContentHit => hit.type !== 'collection').map(
+        (hit) => <SearchResult key={hit.id} hit={hit} />,
+      )}

feat: simplify library home [FC-0062] #1443

Are you sure you want to change the base?

feat: simplify library home [FC-0062] #1443

Conversation

DanielVZ96 commented Oct 27, 2024 • edited Loading

Description

Supporting Information

Testing Instructions

openedx-webhooks commented Oct 27, 2024 • edited Loading

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

Choose a reason for hiding this comment

codecov bot commented Nov 3, 2024 • edited Loading

Codecov Report

DanielVZ96 commented Nov 3, 2024 • edited Loading

bradenmacdonald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpenido Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpenido left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpenido Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielVZ96 commented Oct 27, 2024 •

edited

Loading

openedx-webhooks commented Oct 27, 2024 •

edited

Loading

codecov bot commented Nov 3, 2024 •

edited

Loading

DanielVZ96 commented Nov 3, 2024 •

edited

Loading

rpenido Nov 5, 2024 •

edited

Loading

rpenido Nov 5, 2024 •

edited

Loading