-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: library home page bare bones (TEMP) #37
feat: library home page bare bones (TEMP) #37
Conversation
When lib mode is set to "mixed", both "Libraries" and "Legacy Libraries" tabs are show in the Studio Home. When "Libraries" is clicked, v2 libraries are fetched, when "Legacy Libraries" is clicked, v1 libraries are fetched. When lib mode is set to "v1 only" or "v2 only", only one tab "Libraries" is show and only the respective libraries are fetched when the tab is clicked.
This is to switch between different library modes.
The path updates when selecting tabs, when accessing the url with the path directly it will open its respective tab. Navigating using the browser back/forward buttons is also supported.
049e490
to
9c6e1e6
Compare
This commit is temporary as the current frontend build system in tests doesnt support TS syntax. That should be fixed soon, and this commit should be removed.
This is a temporary commit since there are currently no webpack loaders that support tsx files in the test running. This commit should be removed once that is fixed upstream.
b698d89
to
4391778
Compare
975b8db
to
8b96268
Compare
4391778
to
f8db853
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rpenido Great work on this! Overall it works pretty well, just found a things that need to be fixed and some small nits.
src/index.jsx
Outdated
<Route path="/legacy-libraries" element={<StudioHome />} /> | ||
<Route path="/library/:libraryId" element={<LibraryV2Placeholder />} /> | ||
<Route path="/library/:libraryId/*" element={<LibraryAuthoringPage />} /> | ||
<Route path="/course/:courseId/*" element={<CourseAuthoringRoutes />} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you click on the "Create Library" button when the studio.library_authoring_mfe
flag is disabled, it navigates to the http://apps.local.edly.io:2001/course-authoring/library/create
(which renders the placeholder page as a temporary thing I added in my PR), however that's broken because it assumes create
is a libraryKey and the backend errors out.
Not sure where it should redirect to, maybe a new placeholder "create" page?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rpenido Yes, there is no such screen in the mockups. @bradenmacdonald I guess that's included in another epic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, we missed that. I'll create a story for it. For now, just use the placeholder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the story for it: openedx#1077
<div>You have not added any content to this library yet.</div> | ||
<Button iconBefore={Add}>Add Content</Button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: The text here in both the div and button is missing internationalization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for catching that! Fixed: 4deaea9
return ( | ||
<Stack gap={3}> | ||
<Section title="Recently Modified"> | ||
Recently modified components and collections will be displayed here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Similarly here, internationalize text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed: 4deaea9
src/search-modal/SearchResult.jsx
Outdated
if (libraryAuthoringMfeUrl) { | ||
return `${libraryAuthoringMfeUrl}${urlSuffix}`; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When clicking on the search result it still redirects to the library authoring mfe when the flag is disabled. I think it missing && redirectToLibraryAuthoringMfe
in the check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed: c2bdecf
When lib mode is set to "mixed", both "Libraries" and "Legacy Libraries" tabs are show in the Studio Home. When "Libraries" is clicked, v2 libraries are fetched, when "Legacy Libraries" is clicked, v1 libraries are fetched. When lib mode is set to "v1 only" or "v2 only", only one tab "Libraries" is show and only the respective libraries are fetched when the tab is clicked.
This is to switch between different library modes.
The path updates when selecting tabs, when accessing the url with the path directly it will open its respective tab. Navigating using the browser back/forward buttons is also supported.
This commit is temporary as the current frontend build system in tests doesnt support TS syntax. That should be fixed soon, and this commit should be removed.
This is a temporary commit since there are currently no webpack loaders that support tsx files in the test running. This commit should be removed once that is fixed upstream.
3af4eef
to
8ef4e0c
Compare
8ef4e0c
to
72edfac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rpenido Great work here! Thanks for addressing the issues, things are working well!
Left a few final nits/comments.
index_name: 'studio', | ||
api_key: 'test-key', | ||
}); | ||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extra blank comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed: 388c40e
webpack.dev-tutor.config.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this file slipped thought and got pushed accidentally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed: 388c40e
<SearchField | ||
value={searchKeywords} | ||
placeholder={intl.formatMessage(messages.searchPlaceholder)} | ||
onChange={(value) => setSearchKeywords(value)} | ||
className="w-50" | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The onSubmit
still needs to be defined here, as if you press "Enter" it causes an error onSubmit is not a function
. We can pass in a no-op function () => {}
since the search is already performed onChange
so it doesn't fire again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Fixed: 388c40e
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## yusuf-musleh/lib-v2-tab-studio-home #37 +/- ##
======================================================================
Coverage ? 92.49%
======================================================================
Files ? 718
Lines ? 12745
Branches ? 2809
======================================================================
Hits ? 11789
Misses ? 918
Partials ? 38 ☔ View full report in Codecov by Sentry. |
2e3fa43
to
09e3979
Compare
9ca7038
to
18a8482
Compare
18a8482
to
2d3be09
Compare
This PR adds a new configuration flag that shows/hides tabs in studio home along with some new functionality around to V1 and V2 Libraries. When the new LIBRARY_MODE flag is set to "mixed" (default in dev) it will show "Libraries" and "Legacy Libraries" tabs that correspond to v1 and v2 tabs respectively. When the new LIBRARY_MODE flag is set to "v1 only" (default in production) or "v2 only", only one tab "Libraries" is shown and only the respective libraries are fetched when the tab is clicked. In addition to the above changes, the URL/route now updates when clicking on the tabs, and navigating to it directly would open up that tab as well as a new placeholder page that you will be redirected to when clicking on a v2 library if the library authoring MFE is not enabled.
Closed in favor of: openedx#1076 |
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
Testing Instructions
Private ref: FAL-3753