Skip to content

feat: refactor library routes and add section/subsection tabs #2039

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

Conversation

rpenido
Copy link
Contributor

@rpenido rpenido commented May 28, 2025

Description

This PR adds the "Section" and "Subsections" tabs to the library authoring and refactors our library router hook to fix some ambiguities and solve some bugs.

Additional Information:

Testing Instructions

  • Create a new section/subsection and navigate to the new tabs
  • The tabs should not have the "Type"
  • Check the URL changes while changing tabs and selecting components, collections, units, subsections and sections
  • Check also that you can navigate back and forth between the states
  • Check that you can not stay in a state where the selected component is not in context:
    • Select a "Unit" and click on the "Components" tab, which should deselect the unit
    • Select a "Component" and click on the "Components" tab, which should leave the component selected
  • Opening a Collection/Unit page should open the Collection/Info sidebar, ignoring prior selected components
  • Check the component picker
    • In the Unit Page, click on "Add Existing Content" and you should only see components
    • In the Collection Page, click on"Add Existing Content" and you should see components, units, sections, subsections, but not collections

Private ref: https://tasks.opencraft.com/browse/FAL-4168

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label May 28, 2025
@openedx-webhooks
Copy link

openedx-webhooks commented May 28, 2025

Thanks for the pull request, @rpenido!

This repository is currently maintained by @bradenmacdonald.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are 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.


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.

@rpenido rpenido marked this pull request as draft May 28, 2025 21:52
@github-project-automation github-project-automation bot moved this to Needs Triage in Contributions May 28, 2025
@rpenido rpenido force-pushed the rpenido/fal-4168/library-section-subsection-tabs branch from 439fd90 to 10b54ce Compare May 28, 2025 22:05
Comment on lines 185 to 190
useEffect(() => {
if (!componentPickerMode) {
openInfoSidebar(componentId, collectionId, unitId);
}
}, []);

Copy link
Contributor Author

@rpenido rpenido May 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to SidebarContext

Comment on lines -249 to -259
/*

<FilterByPublished key={
// It is necessary to re-render `FilterByPublished` every time `FilterByBlockType`
// appears or disappears, this is because when the menu is opened it is rendered
// in a previous state, causing an inconsistency in its position.
// By changing the key we can re-render the component.
!(insideCollections || insideUnits) ? 'filter-published-1' : 'filter-published-2'
}
*/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be a temporary/not used code commented

<Routes>
const LibraryLayout = () => (
<Routes>
<Route element={<LibraryLayoutWrapper />}>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the wrapper using proper route nesting instead of a function

Comment on lines -32 to -33
componentId: string | undefined;
setComponentId: (componentId?: string) => void;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed misleading componentId.

Comment on lines 118 to -127
const [collectionId, setCollectionId] = useState(
skipUrlUpdate ? undefined : urlCollectionId || (!selectedItemIdIsUnit ? urlSelectedItemId : undefined),
Copy link
Contributor Author

@rpenido rpenido May 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This state is now only for the collection page

const [collectionId, setCollectionId] = useState(
skipUrlUpdate ? undefined : urlCollectionId || (!selectedItemIdIsUnit ? urlSelectedItemId : undefined),
skipUrlUpdate ? undefined : urlCollectionId,
);
const [unitId, setUnitId] = useState(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This state is now only for the unit page

@rpenido rpenido force-pushed the rpenido/fal-4168/library-section-subsection-tabs branch 8 times, most recently from 5159eda to 3656f84 Compare May 29, 2025 02:09
Copy link

codecov bot commented May 29, 2025

Codecov Report

Attention: Patch coverage is 97.94521% with 3 lines in your changes missing coverage. Please review.

Project coverage is 94.01%. Comparing base (fffa9e2) to head (3adac7d).
Report is 11 commits behind head on master.

Files with missing lines Patch % Lines
src/library-authoring/routes.ts 96.00% 2 Missing ⚠️
...c/library-authoring/collections/CollectionInfo.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2039      +/-   ##
==========================================
+ Coverage   93.99%   94.01%   +0.02%     
==========================================
  Files        1155     1154       -1     
  Lines       24177    24185       +8     
  Branches     5119     5240     +121     
==========================================
+ Hits        22724    22737      +13     
+ Misses       1385     1371      -14     
- Partials       68       77       +9     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@rpenido rpenido force-pushed the rpenido/fal-4168/library-section-subsection-tabs branch 8 times, most recently from 07c061d to 8130606 Compare May 29, 2025 15:15
@rpenido rpenido force-pushed the rpenido/fal-4168/library-section-subsection-tabs branch 3 times, most recently from 4603202 to a45d642 Compare May 29, 2025 19:16
@rpenido rpenido changed the title feat: library section subsection tabs feat: refactor library routes and add section/subsection tabs May 29, 2025
@rpenido rpenido force-pushed the rpenido/fal-4168/library-section-subsection-tabs branch from a45d642 to acf2077 Compare May 29, 2025 19:28
@rpenido rpenido force-pushed the rpenido/fal-4168/library-section-subsection-tabs branch from acf2077 to c6509a9 Compare May 29, 2025 19:39
@rpenido rpenido marked this pull request as ready for review May 29, 2025 20:21
@rpenido rpenido requested a review from pomegranited May 29, 2025 22:39
Copy link
Contributor

@pomegranited pomegranited left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @rpenido , I found some bugs with the route changes, so haven't finished my review.
I'm wondering if we shouldn't keep these changes smaller to reduce regressions?

collectionId?: string,
contentType?: ContentType,
unitId?: string,
doubleClicked?: boolean,
Copy link
Contributor

@pomegranited pomegranited May 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR breaks the "open on double click" behaviour added by #2002 ; they're now opening on single click.

I assume Axim will want Sections and Subsections to open on double click as well (not single click).

Copy link
Contributor Author

@rpenido rpenido May 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @pomegranited!
I didn't remove the double-click behaviour.

Calling navigateTo({ collectionId: "someId" }) now opens the collection page and navigateTo({ selectedItemId: "someId"}) opens the sidebar, so we don't need the double-click anymore.

It was required before because the collectionId could mean "collection open on sidebar" or "collection page open".

Example here:
https://github.com/open-craft/frontend-app-authoring/blob/c6509a92effe6b38d24b868c8ccdf02aa77db405/src/library-authoring/components/ContainerCard.tsx#L205-L219

Can you report to me if it is not working for you?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh.. I understand now. Yep, double clicking is still working as expected, sorry for the noise!

// Overwrite the params with the provided values.
...((selectedItemId !== undefined) && { selectedItemId }),
...((unitId !== undefined) && { unitId }),
...((collectionId !== undefined) && { collectionId }),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a bug when I try to switch back and forth between the All Content and Section / Subsection tabs.
To reproduce:

  1. Start on the main library "All Content" tab
  2. Click on Subsections tab: correct url /subsections
  3. Click on All Content: incorrectly retains the /subsections URL ❌

Same thing happens when switching between "All Content" and "Sections", but not with any of the other tabs.

"All Content" -> "Section" -> "All Content" or

FYI I'm not sure what part of the refactor broke this -- I'm just commenting here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A different bug when switching between Collections and Subsections / Sections:

  1. Start on the main All Content tab.
  2. Click Subsections: correct url /subsections
  3. Click Collections: incorrect url /collections/subsections

Same thing happens with Sections -> Collections.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for that @pomegranited! 😞
I missed adding the routes! Fixed here: 950914d

@rpenido rpenido force-pushed the rpenido/fal-4168/library-section-subsection-tabs branch from 5846373 to 9144414 Compare May 30, 2025 18:39
Comment on lines 192 to 196
useEffect(() => {
// Update the active key whenever the route changes. This ensures that the correct tab is selected
// when navigating using the browser's back/forward buttons because it does not trigger a re-render.
setActiveKey(getActiveKey());
}, [location.key, getActiveKey]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This unreported bug exists on master

Copy link
Contributor

@pomegranited pomegranited left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 The refactoring you did here is really great @rpenido ! Much cleaner now, I think.
And thank you for finding and fixing those bugs with back/forth!

  • I tested this on my tutor dev stack using the PR test instructions, and everything I could think to do.
  • I read through the code
  • I checked for accessibility issues by using my keyboard to navigate
  • Includes documentation -- code comments
  • User-facing strings are extracted for translation

Sorry, something went wrong.

@ChrisChV
Copy link
Contributor

@rpenido Thanks for this refactor! I found an issue on the component picker on the collection Page:

  • It is not possible to switch to other tabs.
  • The same adding library content in a course

https://www.loom.com/share/742a87206c364edbac391640217c158f?sid=d2cef863-f5ca-4cf2-b15a-6f321c4794ac

Copy link
Contributor

@ChrisChV ChrisChV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rpenido Great work! Thanks for this refactor! I left some comments

src/index.jsx Outdated
element={<ComponentPicker extraFilter={['NOT block_type = "unit"']} />}
element={(
<ComponentPicker
extraFilter={['NOT block_type = "unit", "NOT block_type = "section"', 'NOT block_type = "subsection"']}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, shouldn't visibleTabs be added like this? I'm not sure where this case is used to see the Component Picker

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, shouldn't visibleTabs be added like this? I'm not sure where this case is used to see the Component Picker

Sure! That makes sense!
Fixed: a4a6d99

Note: This current code is used on the legacy studio, where an iframe opens the route http://apps.local.edly.io:2001/authoring/component-picker to select components for courses.


// Handle selected item id changes
if (selectedItemId) {
if (selectedItemId.includes(':unit:')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about using getContainerTypeFromId to check if it is a unit and, in the future, check if it is another container?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: 3adac7d

});
});

it('should navigate to the container if double clicked', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rpenido Thanks for these tests!

@pomegranited
Copy link
Contributor

It is not possible to switch to other tabs.

Good catch @ChrisChV ! I neglected to test that detail, so maybe @rpenido can add tests for it here?

…rMode
@rpenido
Copy link
Contributor Author

rpenido commented Jun 2, 2025

It is not possible to switch to other tabs.

Good catch @ChrisChV ! I neglected to test that detail, so maybe @rpenido can add tests for it here?

Sure! Added test here: 3ccddc8

@rpenido
Copy link
Contributor Author

rpenido commented Jun 2, 2025

  • It is not possible to switch to other tabs.

  • The same adding library content in a course

Thanks for catching that! Fixed here: 636653d

@ChrisChV ChrisChV self-requested a review June 2, 2025 18:34
Copy link
Contributor

@ChrisChV ChrisChV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

@ChrisChV ChrisChV merged commit cfb4944 into openedx:master Jun 2, 2025
7 checks passed
@github-project-automation github-project-automation bot moved this from Needs Triage to Done in Contributions Jun 2, 2025
@ChrisChV ChrisChV deleted the rpenido/fal-4168/library-section-subsection-tabs branch June 2, 2025 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants