refactor: split up library context #1539

rpenido · 2024-11-29T17:54:58Z

Description

This PR split the library context into smaller contexts:

LibraryContext
ComponentPickerContext
SidebarContext

Addition Information

Related to Split up LibraryContext (frontend code refactor) #1510

Testing Instructions

Tested library's normal functionality, like adding components, using the sidebar, editing components, adding collections, etc..)

Private ref: FAL-3983

openedx-webhooks · 2024-11-29T17:55:03Z

Thanks for the pull request, @rpenido!

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.

codecov · 2024-11-29T20:03:41Z

Codecov Report

Attention: Patch coverage is 99.09502% with 2 lines in your changes missing coverage. Please review.

Project coverage is 92.96%. Comparing base (c7e2bf9) to head (9b28bb1).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...uthoring/common/context/ComponentPickerContext.tsx	94.11%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1539      +/-   ##
==========================================
+ Coverage   92.94%   92.96%   +0.01%     
==========================================
  Files        1073     1075       +2     
  Lines       21098    21171      +73     
  Branches     4550     4554       +4     
==========================================
+ Hits        19610    19682      +72     
- Misses       1422     1423       +1     
  Partials       66       66

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

rpenido · 2024-12-04T19:30:30Z

src/library-authoring/LibraryLayout.tsx

-      </Routes>
-      <CreateCollectionModal />
-      <ComponentEditorModal />
-      <LibraryTeamModal />


Once the LibraryTeamModal was used only inside the LibraryInfo, I moved the Modal there and removed the methods from the LibraryContext.

navinkarkera

@rpenido Thanks! Looks good. Left a comment about default values for sidebar context.

I tested this: (Tested libraries normal functionality, like adding component, using sidebar, editing component, adding collection etc.)
I read through the code
I checked for accessibility issues
Includes documentation

navinkarkera · 2024-12-10T10:16:53Z

src/library-authoring/common/context/SidebarContext.tsx

+  if (ctx === undefined) {
+    /* istanbul ignore next */
+    throw new Error('useSidebarContext() was used in a component without a <SidebarProvider> ancestor.');
+  }


We need to set a default context instead of raising an error as per this issue: #1510

Thanks for catching this, @navinkarkera!
I changed the LibaryContext and the SidebarContext to return default values here: 829cb5a

It went fine for the SidebarContext. For the LibraryContext, we will need to handle the case where libraryId = undefined at the component level if we don't throw (check the CD/CI errors), and I think it would be better to throw the error at the context level.

What do you think?

@rpenido Yes, I think that LibraryContext doesn't need a default, because of that reason - it's annoying to account for libraryId being undefined everywhere, and it doesn't make sense - libraryId is never undefined on any library page. Since our tests already include that context, I think it's better to make it have no default and throw an error in as it was before.

Thanks @bradenmacdonald!
Done here: c805858

@rpenido Thanks!

navinkarkera · 2024-12-11T04:22:41Z

src/library-authoring/LibraryContent.test.tsx

+      <SidebarProvider>
+        {children}
+      </SidebarProvider>


@rpenido Now that we have default values for sidebar context, the providers can be removed from tests that don't actually use it. For example, this one works without SidebarProvider. Similarly we can simplify other tests, which was the reason for adding default values.

Thanks @navinkarkera! Fixed!

navinkarkera · 2024-12-11T04:26:29Z

src/library-authoring/common/context/SidebarContext.tsx

+  if (ctx === undefined) {
+    /* istanbul ignore next */
+    throw new Error('useSidebarContext() was used in a component without a <SidebarProvider> ancestor.');
+  }


@rpenido Thanks!

ChrisChV

@rpenido Looks good! Great work!

rpenido requested a review from a team as a code owner November 29, 2024 17:54

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

rpenido force-pushed the rpenido/fal-3983-split-up-library-context branch from 00e4b2e to 3662b82 Compare November 29, 2024 19:53

rpenido marked this pull request as draft November 30, 2024 15:02

rpenido added 3 commits December 4, 2024 15:27

refactor: create ComponentPickerContext

175ea87

refactor: move context files

0f451a6

refactor: create SidebarContext

1819bc5

rpenido force-pushed the rpenido/fal-3983-split-up-library-context branch from 7eb7e15 to 1819bc5 Compare December 4, 2024 18:27

rpenido added 3 commits December 4, 2024 15:53

fix: collectionId url consistency

b2f6f4b

refactor: remove LibraryTeamModal from LibraryLayout and LibraryContext

ad6a758

fix: format

36ccc7c

rpenido commented Dec 4, 2024

View reviewed changes

rpenido added 2 commits December 4, 2024 16:50

test: fix tests

27452f7

test: improve coverage

8f89646

navinkarkera approved these changes Dec 10, 2024

View reviewed changes

rpenido added 2 commits December 10, 2024 12:00

Merge branch 'master' into rpenido/fal-3983-split-up-library-context

a4b53a3

fix: add context defaults

829cb5a

rpenido force-pushed the rpenido/fal-3983-split-up-library-context branch from 526b915 to 829cb5a Compare December 10, 2024 18:38

rpenido marked this pull request as ready for review December 10, 2024 21:30

revert: revert LibraryContext defaults

c805858

rpenido force-pushed the rpenido/fal-3983-split-up-library-context branch from 0de1c07 to c805858 Compare December 10, 2024 21:31

navinkarkera reviewed Dec 11, 2024

View reviewed changes

rpenido added 2 commits December 11, 2024 12:54

test: fix typo

eb56bf3

test: remove SidebarProvider

9b28bb1

ChrisChV approved these changes Dec 12, 2024

View reviewed changes

ChrisChV merged commit 69bbeda into openedx:master Dec 12, 2024
7 checks passed

ChrisChV deleted the rpenido/fal-3983-split-up-library-context branch December 12, 2024 18:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: split up library context #1539

refactor: split up library context #1539

rpenido commented Nov 29, 2024 •

edited

Loading

openedx-webhooks commented Nov 29, 2024 •

edited

Loading

codecov bot commented Nov 29, 2024 •

edited

Loading

rpenido Dec 4, 2024

navinkarkera left a comment

navinkarkera Dec 10, 2024

rpenido Dec 10, 2024

bradenmacdonald Dec 10, 2024

rpenido Dec 10, 2024 •

edited

Loading

navinkarkera Dec 11, 2024

navinkarkera Dec 11, 2024

rpenido Dec 11, 2024

navinkarkera Dec 11, 2024

ChrisChV left a comment

refactor: split up library context #1539

refactor: split up library context #1539

Conversation

rpenido commented Nov 29, 2024 • edited Loading

Description

Addition Information

Testing Instructions

openedx-webhooks commented Nov 29, 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?

codecov bot commented Nov 29, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

navinkarkera 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

rpenido Dec 10, 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

ChrisChV left a comment

Choose a reason for hiding this comment

rpenido commented Nov 29, 2024 •

edited

Loading

openedx-webhooks commented Nov 29, 2024 •

edited

Loading

codecov bot commented Nov 29, 2024 •

edited

Loading

rpenido Dec 10, 2024 •

edited

Loading