feat: shareable URLs for library components and searches [FC-0076] #1575
Conversation
to support optional parameters in test paths
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @pomegranited! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo 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:
🔘 Get a green buildIf 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 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:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
pomegranited
force-pushed
the
jill/fal-3984-sharable-urls
branch
from
8fdbe9b to
aa25827
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1575 +/- ##
==========================================
- Coverage 92.96% 92.95% -0.01%
==========================================
Files 1075 1076 +1
Lines 21197 21297 +100
Branches 4560 4515 -45
==========================================
+ Hits 19706 19797 +91
- Misses 1425 1434 +9
Partials 66 66 ☔ View full report in Codecov by Sentry. |
pomegranited
force-pushed
the
jill/fal-3984-sharable-urls
branch
from
5fad981 to
18b4e2d
Compare
* Restructures LibraryLayout so that LibraryContext can useParams() to initialize its componentId/collectionId instead of having to parse complicated route strings. Initialization-from-URL can be disabled for the content pickers by passing skipUrlUpdate to the LibraryContext -- which is needed by the component picker. * Clicking/selecting a ComponentCard/CollectionCard navigates to an appropriate component/collection route given the current page. * Adds useLibraryRoutes() hook so components can easily navigate to the best available route without having to know the route strings or maintain search params. * Moves ContentType declaration to the new routes.ts to avoid circular imports. * Renames openInfoSidebar to openLibrarySidebar, so that openInfoSidebar can be used to open the best sidebar for a given library/component/collection.
pomegranited
force-pushed
the
jill/fal-3984-sharable-urls
branch
from
18b4e2d to
ce4c27e
Compare
This triggers a navigate() call when the searchbox changes, which resets the query counts in testing. So had to reduce the expected counts by 1.
pomegranited
force-pushed
the
jill/fal-3984-sharable-urls
branch
from
d202a8d to
b1fe66e
Compare
Related changes: * adds "manage-team" sidebar action to trigger LibraryInfo's "Manage Team" modal * moves useStateWithUrlSearchParam up to hooks.ts so it can be used by search-manager and library-authoring * splits sidebarCurrentTab and additionalAction out of SidebarComponentInfo -- they're now managed independently as url parameters. This also simplifies setSidebarComponentInfo: we can simply set the new id + key, and so there's no need to merge the ...prev state and new state. * shortens some sidebar property names: sidebarCurrentTab => sidebarTab sidebarAdditionalAction => sidebarAction * test: Tab changes now trigger a navigate() call, which invalidates the within(sidebar) element in the tests. So using screen instead.
pomegranited
force-pushed
the
jill/fal-3984-sharable-urls
branch
from
b1fe66e to
f6b46c4
Compare
| <Routes> | ||
| <Route | ||
| path={ROUTES.COMPONENTS} | ||
| element={context(<LibraryAuthoringPage />)} | ||
| /> | ||
| <Route | ||
| path={ROUTES.COLLECTIONS} | ||
| element={context(<LibraryAuthoringPage />)} | ||
| /> | ||
| <Route | ||
| path={ROUTES.COMPONENT} | ||
| element={context(<LibraryAuthoringPage />)} | ||
| /> | ||
| <Route | ||
| path={ROUTES.COLLECTION} | ||
| element={context(<LibraryCollectionPage />)} | ||
| /> | ||
| <Route | ||
| path={ROUTES.HOME} | ||
| element={context(<LibraryAuthoringPage />)} | ||
| /> | ||
| </Routes> |
There was a problem hiding this comment.
Is this entire <Routes>...</Routes> worth having instead of just putting <LibraryAuthoringPage /> ? I guess it will show a 404 error if none of the routes match? Or does it?
There was a problem hiding this comment.
The advantage of spelling out all of these routes is that it's now very easy to get the :componentId / :collectionId from useParams() and use these as the defaults in LibraryProvider.
I also had to wrap the page element in that context() wrapper so that the routes would be available when LibraryProvider is created -- previously <LibraryLayout> wrapped <LibraryProvider> around the <Routes>, and so <LibraryProvider> couldn't use them.
|
|
||
| export const ROUTES = { | ||
| COMPONENTS: '/components/:componentId?', | ||
| COLLECTIONS: '/collections/:collectionId?', |
There was a problem hiding this comment.
Why is there a collectionId in COLLECTIONS? A comment would be helpful. Same question for HOME.
pomegranited
force-pushed
the
jill/fal-3984-sharable-urls
branch
from
fa8783d to
f6bf086
Compare
Related features: 1. We can filter on more than one tag, so the search field needs to store an Array of values which still need to be sanitised. This change adds useListHelpers to assist with the parsing and validating of an Array of Typed values. 2. SearchManager takes a skipUrlUpdate property which should disable using search params for state variables. Our "sort" parameters respected this, but none of the other search parameters did. So this change also adds a wrapper hook called useStateOrUrlSearchParam that handles this switch cleanly. This feature also revealed two bugs fixed in useStateWithUrlSearchParam: 1. When the returnSetter is called with a function instead of a simple value, we need to pass in previous returnValue to the function so it can generate the new value. 2. When the returnSetter is called multiple times by a single callback (like with clearFilters), the latest changes to the UrlSearchParams weren't showing up. To fix this, we had to use the location.search string as the "latest" previous url search, not the prevParams passed into setSearchParams, because these params may not have the latest updates.
pomegranited
force-pushed
the
jill/fal-3984-sharable-urls
branch
from
f6bf086 to
37370d2
Compare
Combine blockTypesFilter + problemTypesFilter into a single state variable called typesFilter, and store selected block + problem types in the url search parameters. These two states are heavily interdependent, and so the FilterByBlockType code was quite complicated. Then storing these states as url search params caused re-renders that disrupted the delicate dependencies between these two states. This change stores both these type lists using a single TypesFilterData class to simplify the code and avoid bugs when updating their states.
There was a problem hiding this comment.
@pomegranited Everything works nicely! Great work!.
Just found a small bug: On collections page, select any collection so that its sidebar appears (this appends st=manage to query params), now click on Library Info button and then on Manage Access button. This opens up Library Team modal and adds sa=manage-team query param to url. On refreshing page, modal is not displayed, most probably due to presence of st=manage query param in url. We should remove it on clicking Library info button or display of library sidebar.
Using the Component card menu, select Add to Collection. Note that ?sa=jump-to-add-collections is in the query string, and this is preserved when you navigate between components.
Should we also add this parameter when author clicks on Add to Collection button in the component sidebar?
| } = useLibraryContext(); | ||
| const { openInfoSidebar, sidebarComponentInfo } = useSidebarContext(); | ||
|
|
||
| const { insideCollections, insideComponents, navigateTo } = useLibraryRoutes(); | ||
|
|
||
| // The activeKey determines the currently selected tab. | ||
| const [activeKey, setActiveKey] = useState<ContentType>(ContentType.home); |
There was a problem hiding this comment.
NIT: We don't need useEffect as we can use a function to initialize activeKey state.
Something like this should work.
const getActiveKey = () => {
if (componentPickerMode) {
return ContentType.home;
}
if (insideCollections) {
return ContentType.collections;
}
if (insideComponents) {
return ContentType.components;
}
return ContentType.home;
};
// The activeKey determines the currently selected tab.
const [activeKey, setActiveKey] = useState<ContentType>(getActiveKey);| }: { | ||
| fromString: FromStringFn<Type>, | ||
| toString: ToStringFn<Type>, | ||
| separator?: string; |
There was a problem hiding this comment.
NIT: We can make use of browsers capability to convert query params with same name to an array like object instead of manually adding a separator and splitting it. useSearchParams from react-router returns URLSearchParams which has getAll that returns an array.
Description
Describe what this pull request changes, and why. Include implications for people using this change.
Design decisions and their rationales should be documented in the repo (docstring / ADR), per
Useful information to include:
Use cases covered:
Supporting information
See #1499
Private-ref: FAL-3984
Testing instructions
Recommend going commit-by-commit to do the code review, since this is a pretty big change.
This change involved some route additions and changes, and so to manually test them, please refresh your browser page after each step, and ensure you see exactly the same page -- including selected tabs, sidebar, and sidebar tabs. Please also check that using the back/forth buttons navigate smoothly between pages as expected, but that changing the library search parameters doesn't append to the browser history.
/librariesand create or select an existing library from the list."All Content" should still be the default tab you see.
Note that the component picker still works as expected.
Note that the URL changes to
/library/:libraryId/component/:componentId, and the component is visible in the sidebar.Note that the URL changes to reflect the new componentId.
Note that the URL changes to
/library/:libraryId/:collectionId, and the collection is visible in the sidebar.Note that the URL changes to reflect the new collectionId.
Note that the sidebar tab is preserved (where possible) when navigating between components or collections.
Clicking Library Info should reset the sidebar back to the Library.
Note that you remain in the selected tab when switching between selected components/collections.
Note that the URL changes to
/library/:libraryId/collection/:collectionId, and the collection is visible in the sidebar.Note that the URL changes to
/library/:libraryId/collection/:collectionId/:componentId, and the component is visible in the sidebar.Note that the sidebar tab is preserved (where possible) when navigating between components.
Clicking Collection Info should reset the sidebar back to the Collection.
Note that your keywords are stored in a
qquery string parameter, and preserved on page refresh.Note that these are store in the query string, and cleared when de-selecting or clicking Clear Filters.
Note that
?sa=manage-teamis the query string, and refreshing the page re-opens the Library Team modal.Note that
?sa=jump-to-add-collectionsis in the query string, and this is preserved when you navigate between components.Clicking "Confirm" or "Cancel" removes this parameter and closes the collection manager.
Other information
TODOs in the SearchManager code about sanitizing parameters, because sanitising user input is good practice. But I'm not sure how best to sanitise things like tags and search keywords?clearFilterson multiple search filters that are now using theuseStateWithUrlSearchParamhook -- more details in this comment. Advice or alternatives welcome!