feat: Add plugin slots for progress page components

[xitij2000]

Adds a slot for different components in the progress tab to allow them to be overridden with custom components.

The main aim here is to allow a client to support enabling/disabling individual components on a per-course basis. If this is a feature that will be valuable to the broader community we can change the implementation here to directly support that as well.

This change currently allows overriding individual components. If this is too granular we can also look into creating a slot for the entire page, however that will require more complexity and drift for the overriding component if it just wants to make minor changes.

[openedx-webhooks]

Thanks for the pull request, @xitij2000!

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/committers-frontend-app-learning. 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:

• Overview of Review Process for Community Contributions
• Pull Request Status Guide
• Making changes to your pull request

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.

[xitij2000]

This PR currently doesn't include any screenshots. I will add them before the PR is final depending on the direction of the review.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.87%. Comparing base (dafdcad) to head (5721f77).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1496      +/-   ##
==========================================
+ Coverage   89.81%   89.87%   +0.05%     
==========================================
  Files         325      331       +6     
  Lines        5598     5631      +33     
  Branches     1396     1357      -39     
==========================================
+ Hits         5028     5061      +33     
- Misses        554      555       +1     
+ Partials       16       15       -1     

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

[itsjeyd]

@xitij2000 I marked this PR as ready for review assuming that it doesn't change any existing user-facing behavior. Let me know if that's wrong, please (it would need to go through product review in that case).

CC @openedx/committers-frontend-app-learning

[bradenmacdonald]

@xitij2000 This makes sense to me at a high level. Would you mind adding some screenshots, as you suggested?

@arbrandes Do we have any specific guidelines around where we'll accept slots in the UI, or just use my best judgement?

[xitij2000]

@xitij2000 This makes sense to me at a high level. Would you mind adding some screenshots, as you suggested?

@arbrandes Do we have any specific guidelines around where we'll accept slots in the UI, or just use my best judgement?

Additionally, if we do want component-level slots (which I think we do since there is a slot to add contents to a unit title), then perhaps we can structure them in some way in he plugin-slots folder? Otherwise, as the number of slots grows it will become unwieldy.

[itsjeyd]

@xitij2000 @bradenmacdonald I'm a little unclear on the current status of this PR.

Are there any blockers to starting engineering review?

[bradenmacdonald]

@itsjeyd I don't think so. I just hoping to hear from @arbrandes.

[itsjeyd]

@bradenmacdonald OK, got it.

[itsjeyd]

@bradenmacdonald Do you want to explicitly request a review from him, maybe? That might help with getting this PR unstuck.

CC @xitij2000

[bradenmacdonald]

@brian-smith-tcril Could I get your thoughts on this PR? I think it sounds like a good idea, and Adolfo suggested we might want to backport it to Sumac, which I agree with.

[brian-smith-tcril]

Thank you so much for this PR!

Overall I think these are great extension points to have, and I'm super happy to see these documented with screenshots and everything!

I left a few comments with questions and suggestions. Most are quite small, but there's one larger comment that might spark conversation.

[brian-smith-tcril]

I'm not opposed to having courseId in pluginProps, but my general feeling is we should only add pluginProps when we know that there is a use case that requires them. Adding something to pluginProps makes it part of the plugin API, meaning removing it would require going through a DEPR process. Do you have an example use case in mind where courseId would be required in this slot?

[xitij2000]

I agree. I will remove them from here since the courseId should be unambiguous in these slots and can probably be fetched from the store.

[bradenmacdonald]

I will eventually want to refactor every page like this in the Learning MFE to not use redux and to use React Context + React Query instead. So if you do need course ID, I would prefer plugins don't fetch it from the store, because that's implicitly making the Redux store part of the API contract here. Our redux stores tend to be messy and buggy because they don't even have a contract within each MFE (they aren't typed using TypeScript), and I really don't want them to become part of the plugin contract, whether officially or unofficially.

In general, my personal view is that plugin slots should pass any props that come from the URL, so that they know exactly which page they're on (in this case it would be the course ID), and any other data that they need should be loaded via small usages of React Query (which should be de-duplicated if any other places on the page need the same data).

In this case, the default content of the plugin slot

does need the course ID (e.g. "Dates" links to /learning/course/:courseId/dates), so I think it makes sense to make that course ID available to any other plugins filling the slot.

[xitij2000]

I think perhaps it makes sense to standardise some kind of common context across all MFEs then and plugin can fetch data from there without needing props. I think courseId / learningContextId probably belongs in the context as it is by definition the context in which each component is rendered. At the bare minimum I think the shared common context should have the contextId, username etc. If the username is missing then a user isn't logged in and if the contextId is missing then it doesn't apply (for instance in the profile page or learner dashboard).

Or it could be as simple as having hooks like useContextId which can then get it from the store right now and from the shared context or whatever mechanism we have later. It seems like too common a usecase here.

The removal of the courseId is in an isolated commit so I can revert easily, but, how about I instead add a useContextId hook that is designed for any component or plugin to use? If it looks good we can standardise something like that as a common API>

[bradenmacdonald]

I think perhaps it makes sense to standardise some kind of common context across all MFEs then and plugin can fetch data from there without needing props. I think courseId / learningContextId probably belongs in the context as it is by definition the context in which each component is rendered.

Totally agree with this.

Or it could be as simple as having hooks like useContextId which can then get it from the store right now and from the shared context or whatever mechanism we have later. It seems like too common a usecase here.

Great idea. Maybe just call it "useLearningContextId" since "Context" is ambiguous to me.

[brian-smith-tcril]

I'd like to hear others thoughts on having the ProgressTabCertificateStatusSlot in 2 places here. The way this is written now would require plugin authors utilizing the ProgressTabCertificateStatusSlot to account for the slot being in the side panel or main body depending on the width of the viewport.

I see a few options here:

• A: Keep the slots as written in this PR

  • Requires plugin authors to account for slots being in different places based on viewport width

• B: Split ProgressTabCertificateStatusSlot into ProgressTabCertificateStatusMainBodySlot and ProgressTabCertificateStatusSidePanelSlot

  • Requires site operators to put a plugin in 2 slots instead of 1 (even if it's the same plugin)

• C: Switch to just having a MainBody slot and a SidePanel slot instead of granular slots for each component in the body/panel

  • This would remove quite a bit of flexibility in customization

• D: Some combination of (A or B) and C

  • This would mean keeping the granular slots, but also wrapping those slots in a bigger slot - similar to how frontend-component-header has a DesktopHeaderSlot that wraps the entire desktop header, while the desktop header component contains more granular slots such as DesktopUserMenuSlot

  I think I lean towards B, with a possibility of adding bigger slots later if needed. I am, however, very open to other opinions on the matter, and would love to hear other options I haven't thought of that others have.

[bradenmacdonald]

B seems reasonable to me.

[xitij2000]

Perhaps there is another option, which is to pass the slot position to component?

i.e. one of the props can be the "placement" of the content, which could be "mainbody" or "sidebar".

I imagine anyone creating a plugin for this pair of slots would either use the same component or have a single component dynamically adjust to the placement. By explicitly providing this value we can ensure that the plugin author will follow the same logic as the app even if that changes over time.

Otherwise, I agree that the two slot approach looks good, and I will adapt the code accordingly. I'll push the approach using the "placement" prop since it's a small change, but will switch to the two component approach if that seems better.

[xitij2000]

@brian-smith-tcril I've updated the slots to remove the courseId. I've updated the examples to pull the course ID from the state so that I wouldn't have to update the images though. I hope that's okay!

[brian-smith-tcril]

I'm not sure how I feel about having placement as a pluginProp.

I think 2 slots provides more flexibility. With 2 slots, site operators can use different PLUGIN_OPERATIONS for each, set keepDefault differently for each, or set a different priority for each.

With placement as a pluginProp that flexibility is lost.

That being said, I don't know if that flexibility is something we want. My current feeling is, "yes, we should provide the flexibility that comes with using 2 slots" - but I'm open to other opinions on the matter.

[xitij2000]

You are right, I did not consider that someone might want to apply different plugin operations depending on placement.

On further thought I do have another issue with "placement" which is that currently it's not standardised. I do think there could be certain slots that would apply to multiple places in an app. In that case, it would be useful to have a standardised terminology for what it means for the placement to be in the "main body" or "sidebar" etc.

I'm wondering then if the logic of which version should be hidden should also be moved to the contents inside the plugin? For instance someone might want to always show it in the sidebar and others always in the main body?

[xitij2000]

I've updated the PR to use two components and moved the logic of hiding or showing the component to the slots so that a plugin can choose to use different logic.

[brian-smith-tcril]

Overall I really like how this has come together! I'd still like to have a bigger conversation about best practices for getting data into plugins (context vs props etc.), but I don't think that should block merging this.

It looks like CI is failing commitlint and some tests, once those are passing I'll give this a ✔️!

[xitij2000]

@brian-smith-tcril I've fixed the test issues, and the commitlint issues. Thanks for your thorough review!

I feel starting with fewer props and adding more later is less disruptive so keeping courseId out of props seems good for now, and if things change, it will be easy to add it back without hurting existing plugins.

[brian-smith-tcril]

I feel starting with fewer props and adding more later is less disruptive

I 100% agree!

Thanks for all the work on this! It turned out great!

[pdpinch]

Are there screenshots and/or a description of this PR that I can share with a product manager or instructional designer?

We have a backlog of pedagogical issues with the progress page, and I'm wondering if we can leverage the pluginslots to get the desired behavior.

[xitij2000]

Are there screenshots and/or a description of this PR that I can share with a product manager or instructional designer?

We have a backlog of pedagogical issues with the progress page, and I'm wondering if we can leverage the pluginslots to get the desired behavior.

This doesn't change the UI at all, so it's hard to screenshot as-is, but I'm adding a screenshot that highlights all the elements that can now be altered with slots.

[itsjeyd]

@bradenmacdonald Did you want to give this another look before merging?

If not, it looks like the changes will be ready to go after another rebase.

[bradenmacdonald]

@itsjeyd Nope, this is good to merge without further review from me. Thanks.