feat: library home page bare bones [FC-0059] #1076

rpenido · 2024-06-06T18:49:07Z

Description

This PR adds a "Bare Bones" version of the Library Home and updates the SearchResult component to redirect to this new page.

More Info

Closes: Library Home Page - "Bare Bones" #1036
Depends on: feat: Add lib v2/legacy tabs in studio home #1050

Testing Instructions

Run your local stack on this branch
Make sure you have the studio.library_authoring_mfe waffle flag DISABLED
Go to the Studio Home and check the "Libraries" tab
Click on one library with components
Check if the component count is shown in the last card and the "Components" tab
Use the keyword search box and check if the component count is filtered accordingly
Navigate through the Home, Components and Collections tab and check if you can navigate back using the "Back" button from the brownser
Go back to the Studio Home and open a library WITHOUT components
You should see a message with a button that will allow the user to add new components (not implemented yet)
Go back to the Studio Home and open the search modal
Find a library v2 component and click on it. It should redirect to the new Library Home.

Private ref: FAL-3753

Settings

EDX_PLATFORM_REPOSITORY: https://github.com/open-craft/edx-platform.git
EDX_PLATFORM_VERSION: taxonomy-sandbox-20240614

GROVE_REDIRECTS:
 - domain: {{ LMS_HOST }}
 - domain: cms.{{ LMS_HOST }}
 - domain: app.{{ LMS_HOST }}
 - domain: meilsearch.{{ LMS_HOST }}

PLUGINS:
- mfe
- grove
- s3
- meilisearch

Tutor requirements

git+https://github.com/overhangio/tutor.git@nightly
git+https://github.com/overhangio/tutor-discovery.git@nightly
git+https://github.com/overhangio/tutor-ecommerce.git@nightly
git+https://github.com/overhangio/tutor-xqueue.git@nightly
git+https://github.com/overhangio/tutor-forum.git@nightly
git+https://gitlab.com/opencraft/dev/tutor-contrib-grove.git@main

git+https://github.com/open-craft/tutor-mfe.git@7d1e1e0dad2c0e4247a4fd8280c257f13ed28432
git+https://github.com/open-craft/tutor-contrib-s3.git@04c7491442fa231e79840995f1f8707eed6353fd

openedx-webhooks · 2024-06-06T18:49:12Z

Thanks for the pull request, @rpenido! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

codecov · 2024-06-06T20:04:29Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.68%. Comparing base (292663a) to head (624309d).
Report is 680 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1076      +/-   ##
==========================================
+ Coverage   92.67%   92.68%   +0.01%     
==========================================
  Files         696      693       -3     
  Lines       12392    12347      -45     
  Branches     2714     2694      -20     
==========================================
- Hits        11484    11444      -40     
+ Misses        876      872       -4     
+ Partials       32       31       -1

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

rpenido · 2024-06-07T20:14:16Z

src/CourseAuthoringPage.jsx

-const AppHeader = ({
-  courseNumber, courseOrg, courseTitle, courseId,
-}) => (
-  <Header
-    courseNumber={courseNumber}
-    courseOrg={courseOrg}
-    courseTitle={courseTitle}
-    courseId={courseId}
-  />
-);
-
-AppHeader.propTypes = {
-  courseId: PropTypes.string.isRequired,
-  courseNumber: PropTypes.string,
-  courseOrg: PropTypes.string,
-  courseTitle: PropTypes.string.isRequired,
-};
-
-AppHeader.defaultProps = {
-  courseNumber: null,
-  courseOrg: null,
-};
-


This component was just passing parameters to <Header />.

rpenido · 2024-06-07T20:14:43Z

src/header/Header.jsx

+  contentId,
+  org,
+  number,
+  title,
  isHiddenMainMenu,
+  isLibrary,
 }) => {


We are using the same Header for courses and libraries now.

rpenido · 2024-06-07T20:17:11Z

src/header/Header.jsx

      />
      { meiliSearchEnabled && (
        <SearchModal
          isOpen={isShowSearchModalOpen}
-          courseId={courseId}
+          courseId={isLibrary ? undefined : contentId}


ToDo: We can make the search modal aware of whether the current library is the same way we do with courses. The search will work out of the box, but we need to revisit the page to change some wording ("all courses", "current course", etc..)

For now, we want the modal to only search courses and we're not going to show "Search all Courses and Libraries" even though it's possible.

To search libraries, users will have to click on a specific library and use the new search UI we're building, which is not in a modal.

Ack! @bradenmacdonald

For now, we want the modal to only search courses and we're not going to show "Search all Courses and Libraries" even though it's possible.

It is worth to mention that currently we show library blocks in the modal and redirect to the parent library if you click on a hit.

Would you like me to include a fix in this PR to limit it only to courses?

Right, yes please - let's keep that functionality in the code but for now add an additional filter to limit the search modal results to courses. Thanks.

src/search-modal/data/apiHooks.js

rpenido · 2024-06-07T20:20:47Z

src/library-authoring/data/apiHook.js

+/**
+ * Hook to fetch the count of components and collections in a library.
+ * @param {string} libraryId - The ID of the library to fetch.
+ * @param {string} searchKeywords - Keywords to search for.
+ */
+export const useLibraryComponentCount = (libraryId, searchKeywords) => {
+  // Meilisearch code to get Collection and Component counts
+  const { data: connectionDetails } = useContentSearchConnection();
+
+  const indexName = connectionDetails?.indexName;
+  const client = React.useMemo(() => {
+    if (connectionDetails?.apiKey === undefined || connectionDetails?.url === undefined) {
+      return undefined;
+    }
+    return new MeiliSearch({ host: connectionDetails.url, apiKey: connectionDetails.apiKey });
+  }, [connectionDetails?.apiKey, connectionDetails?.url]);
+
+  const libFilter = `context_key = "${libraryId}"`;
+
+  const { totalHits: componentCount } = useContentSearchResults({
+    client,
+    indexName,
+    searchKeywords,
+    extraFilter: [libFilter], // ToDo: Add filter for components when collection is implemented
+  });
+
+  const collectionCount = 0; // ToDo: Implement collections count
+
+  return {
+    componentCount,
+    collectionCount,
+  };
+};


I created this hook to get only the component count for the library. Maybe we should replace this with another instance of the SearchContext when we add other filters. @ChrisChV @bradenmacdonald

Yes, when we get there what I want to do is move most of the "generic" search code out of search-modal into somewhere like generic/search/ and then we can use it in both search-modal/SearchModal.tsx and src/library-authoring/LibraryHome.tsx etc.

@bradenmacdonald
I was thinking of exporting this directly from our search-modal feature (as I did in this file here). I know the current name is not the best, but it would be our search-engine feature that exports both the modal and the provider to use the search engine elsewhere. Maybe rename this feature to search-engine to make it explicit.

Or do you think having two features (search-modal and search-engine) would work best?

And, just to confirm: we are moving to use the search index as the primary source for listing content, right?

We might need to move some indexing tasks to run sync because if we add or remove a library, we could refetch before the index update and end up with out of sync data.

Yes, exactly. When we make changes to libraries / library content, we can update the search index synchronously.

It's fine to export from search-modal for now, but I do think it's better to separate the library (search manager) from the two use cases: library home UI, and search modal.

yusuf-musleh · 2024-06-19T12:37:58Z

@rpenido I left a comment on the last commit here

open-craft-grove · 2024-06-19T14:25:13Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

open-craft-grove · 2024-06-19T14:40:34Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

open-craft-grove · 2024-06-19T18:48:43Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

open-craft-grove · 2024-06-19T18:53:16Z

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

yusuf-musleh

👍 @rpenido Looks good to me! Great work!

I tested this: I followed the testing instructions in the PR, and everything is working as expected.
I read through the code
I checked for accessibility issues
~~Includes documentation~~

xitij2000

Just a few minor nits other than that it's good to go!

xitij2000 · 2024-07-09T08:14:59Z

src/header/Header.tsx

-  ];
-  const outlineLink = `${studioBaseUrl}/course/${courseId}`;
+  ] : [];
+  const outlineLink = !isLibrary ? `${studioBaseUrl}/course/${contentId}` : `/course-authoring/library/${contentId}`;


The /course-authoring/ part is configurable. I'd recommend using generatePath from react router for this.

Done: 9937a59

xitij2000 · 2024-07-09T09:06:43Z

src/library-authoring/EmptyStates.tsx

+    <FormattedMessage {...messages.noComponents} />
+    <Button iconBefore={Add}>
+      <FormattedMessage {...messages.addComponent} />


This isn't specific to this code, but I've seen this pattern a lot and I don't personally see the value in it.

To me the advantage of using FormattedMessage is that you can see the actual text in the react code instead of looking at the messages file. In this case it's better to use intl.formatMessage instead. FormattedMessage just uses useIntl internally anyway.

Are you saying to write { intl.formatMessage(messages.addComponent) } ? That's totally fine, but requires changing this function to have a statement body to be able to include const intl = useIntl() in scope, so writing it this way uses one less line of code, and I think is just as simple.

xitij2000 · 2024-07-09T09:09:27Z

src/library-authoring/LibraryAuthoringPage.tsx

+const TAB_LIST = {
+  home: '',
+  components: 'components',
+  collections: 'collections',
+};
+


Since we're using TypeScript now, this could be an enum?

Great! Done: 3b2a6ad

src/header/Header.tsx

bradenmacdonald · 2024-07-09T09:51:26Z

src/header/Header.tsx

        isHiddenMainMenu={isHiddenMainMenu}
        mainMenuDropdowns={mainMenuDropdowns}
        outlineLink={outlineLink}
-        searchButtonAction={meiliSearchEnabled && openSearchModal}
+        searchButtonAction={meiliSearchEnabled ? openSearchModal : undefined}


Let's hide the search button for libraries please.

I don't think we agree here. Shouldn't we have the "same" header on all pages? I thought the search modal was a quick jump to content from anywhere in the course authoring. Could you elaborate a bit more?

Currently, the search icon appears in "all" pages (i.e., Taxonomy pages). We would need to create a new parameter for our header to hide it. Let me know if you think we want to go to this route!

Well I think people will be confused that the search icon on the library pages doesn't search libraries. But we can leave it for now and see what the product/UX folks say later when they review and try it out on the sandbox. It's a bit of an issue that the UX for searching libraries and the UX for searching courses are so completely different.

bradenmacdonald · 2024-07-09T09:58:33Z

This is a weird bug, but if I go to the "Libraries" tab, then click on any library, then click back, the "New Library" button disappears.

Edit: also, just refreshing the Studio Home makes the buttons disappear?

Edit: I think this bug is on master too, actually

src/studio-home/StudioHome.jsx

src/library-authoring/LibraryAuthoringPage.test.tsx

bradenmacdonald · 2024-07-09T10:18:23Z

src/library-authoring/LibraryAuthoringPage.test.tsx

+      </QueryClientProvider>
+    </IntlProvider>
+  </AppProvider>
+);


Not for this PR, but we should really make a re-usable RootWrapper for all our test files instead of writing this out in each one.

Thank you @bradenmacdonald . I was a bit divided about this before.
This structure explicitly states what is needed for this particular component, but it also adds a lot of boilerplate to the tests.

For the next tests, I will add some helpers for this (and the meilisearch stuff).

src/library-authoring/LibraryAuthoringPage.tsx

bradenmacdonald · 2024-07-09T10:26:15Z

src/library-authoring/LibraryHome.tsx

+
+  return (
+    <Stack gap={3}>
+      <Section title="Recently Modified">


These titles need i18n

Fixed: 2a63186

bradenmacdonald · 2024-07-09T10:28:23Z

src/library-authoring/data/apiHook.ts

+/**
+ * Hook to fetch the count of components and collections in a library.
+ */
+export const useLibraryComponentCount = (libraryId: string, searchKeywords: string) => {


This is temporary right? In the future we should have a <SearchContextProvider> on the Library Home page, and there should be no need to initialize a Meilisearch client manually.

Yes! This was refactored here: #1141

bradenmacdonald · 2024-07-09T10:30:57Z

src/search-modal/index.js

@@ -0,0 +1,3 @@
+// @ts-check


Just make this file .ts please. Or do we really need this file?

I think this is good for helping us avoid inter-feature import. We should not import from ./seach-modal/Something. We should always import from explicitly exported members from ./search-modal

rpenido · 2024-07-09T15:13:21Z

This is a weird bug, but if I go to the "Libraries" tab, then click on any library, then click back, the "New Library" button disappears.

Edit: also, just refreshing the Studio Home makes the buttons disappear?

Edit: I think this bug is on master too, actually

This seems to fix the issue: 01e817a

rpenido · 2024-07-10T07:23:16Z

Thank you for the reviews @bradenmacdonald and @xitij2000!
I addressed the issues and added a comment about the search button in the head bar.

This is ready for another round.

bradenmacdonald · 2024-07-10T08:16:43Z

Nice work on addressing those comments, @rpenido. @xitij2000 did you want to look at this again before we merge it?

openedx-webhooks · 2024-07-10T12:20:03Z

@rpenido 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

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

rpenido force-pushed the rpenido/fal-3753-library-home-page-bare-bones branch 3 times, most recently from d212fa9 to d1b529c Compare June 6, 2024 19:55

rpenido force-pushed the rpenido/fal-3753-library-home-page-bare-bones branch 11 times, most recently from 8ef4e0c to 72edfac Compare June 7, 2024 18:59

rpenido commented Jun 7, 2024

View reviewed changes

src/search-modal/data/apiHooks.js Outdated Show resolved Hide resolved

rpenido commented Jun 7, 2024

View reviewed changes

rpenido added the create-sandbox open-craft-grove should create a sandbox environment from this PR label Jun 19, 2024

rpenido force-pushed the rpenido/fal-3753-library-home-page-bare-bones branch 2 times, most recently from 18a8482 to 2d3be09 Compare June 20, 2024 02:22

fix: add intl to tab names

905dbb5

rpenido force-pushed the rpenido/fal-3753-library-home-page-bare-bones branch from d197577 to 905dbb5 Compare July 5, 2024 16:17

yusuf-musleh approved these changes Jul 9, 2024

View reviewed changes

xitij2000 approved these changes Jul 9, 2024

View reviewed changes

bradenmacdonald reviewed Jul 9, 2024

View reviewed changes

src/header/Header.tsx Outdated Show resolved Hide resolved

bradenmacdonald reviewed Jul 9, 2024

View reviewed changes

src/studio-home/StudioHome.jsx Outdated Show resolved Hide resolved

bradenmacdonald requested changes Jul 9, 2024

View reviewed changes

rpenido added 3 commits July 9, 2024 16:45

fix: rename contentId -> contextId

208c0e6

fix: use ContentLibrary type

2764aa0

refactor: change TAB_LIST to enum and remove currentTab state

3b2a6ad

rpenido force-pushed the rpenido/fal-3753-library-home-page-bare-bones branch from 9565a4b to 3b2a6ad Compare July 9, 2024 15:02

fix: create course and library button missing on refresh

01e817a

rpenido added 8 commits July 9, 2024 17:15

refactor: move index.js -> index.ts

3fe2eb4

fix: add intl to section titles

2a63186

fix: use navigate to go to create library page

4df024e

fix: typo

b0f6310

fix: remove static course-authoring/ path

9937a59

chore: trigger CI

0525ea7

fix: removed unused import

d6eee6f

test: fix tests after navigation change

624309d

bradenmacdonald approved these changes Jul 10, 2024

View reviewed changes

bradenmacdonald merged commit f60ddb5 into openedx:master Jul 10, 2024
6 checks passed

rpenido deleted the rpenido/fal-3753-library-home-page-bare-bones branch July 12, 2024 12:08

mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Sep 19, 2024

feat: library home page bare bones [FC-0059] #1076

feat: library home page bare bones [FC-0059] #1076

Conversation

rpenido commented Jun 6, 2024 • edited Loading

Description

More Info

Testing Instructions

openedx-webhooks commented Jun 6, 2024 • edited Loading

codecov bot commented Jun 6, 2024 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

rpenido Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yusuf-musleh commented Jun 19, 2024

open-craft-grove commented Jun 19, 2024

open-craft-grove commented Jun 19, 2024

open-craft-grove commented Jun 19, 2024

open-craft-grove commented Jun 19, 2024

yusuf-musleh left a comment

Choose a reason for hiding this comment

xitij2000 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpenido Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradenmacdonald commented Jul 9, 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

Choose a reason for hiding this comment

rpenido commented Jul 9, 2024

rpenido commented Jul 10, 2024

bradenmacdonald commented Jul 10, 2024

openedx-webhooks commented Jul 10, 2024

rpenido commented Jun 6, 2024 •

edited

Loading

openedx-webhooks commented Jun 6, 2024 •

edited

Loading

codecov bot commented Jun 6, 2024 •

edited

Loading

rpenido Jun 19, 2024 •

edited

Loading

rpenido Jul 9, 2024 •

edited

Loading

bradenmacdonald commented Jul 9, 2024 •

edited

Loading