Instructor tabs #32

diana-villalvazo-wgu · 2025-10-09T20:32:06Z

Description

Components basic implementation for instructor tabs.

Demo

Screen.Recording.2025-10-09.at.2.30.20.p.m.mov

Support Information

Closes #23

openedx-webhooks · 2025-10-09T20:32:13Z

Thanks for the pull request, @diana-villalvazo-wgu!

This repository is currently maintained by @openedx/committers-frontend-app-instruct.

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.

Details

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 · 2025-10-09T20:33:15Z

Codecov Report

❌ Patch coverage is 0% with 86 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.96%. Comparing base (5b0d465) to head (7b256f4).
⚠️ Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
src/instructorTabs/InstructorTabs.tsx	0.00%	36 Missing ⚠️
src/routes.tsx	0.00%	25 Missing ⚠️
...rc/slots/instructorTabsSlot/instructorTabsSlot.tsx	0.00%	4 Missing ⚠️
src/pageWrapper/PageWrapper.tsx	0.00%	3 Missing ⚠️
src/certificates/CertificatesPage.tsx	0.00%	2 Missing ⚠️
src/courseTeam/CourseTeamPage.tsx	0.00%	2 Missing ⚠️
src/dataDownloads/DataDownloadsPage.tsx	0.00%	2 Missing ⚠️
src/dateExtensions/DateExtensionsPage.tsx	0.00%	2 Missing ⚠️
src/enrollments/EnrollmentsPage.tsx	0.00%	2 Missing ⚠️
src/grading/GradingPage.tsx	0.00%	2 Missing ⚠️
... and 4 more

Additional details and impacted files

@@            Coverage Diff             @@
##            main      #32       +/-   ##
==========================================
+ Coverage   0.00%   54.96%   +54.96%     
==========================================
  Files          5       39       +34     
  Lines          7      322      +315     
  Branches       0       78       +78     
==========================================
+ Hits           0      177      +177     
- Misses         7      145      +138

☔ 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.

bradenmacdonald · 2025-10-09T21:23:00Z

Are we not making the tabs plugin-based?

diana-villalvazo-wgu · 2025-10-09T23:48:28Z

Are we not making the tabs plugin-based?

What do you mean by plugin-based? 🤔

bradenmacdonald · 2025-10-10T21:19:23Z

Aren't we going to make the tab bar a plugin slot, and each tab ("Course Info", "Enrollments", "Course Team", etc.) a plugin? So that people can make custom plugins to add new tabs to the instructor dashboard without having to fork and modify this MFE?

diana-villalvazo-wgu · 2025-10-31T16:35:50Z

Aren't we going to make the tab bar a plugin slot, and each tab ("Course Info", "Enrollments", "Course Team", etc.) a plugin? So that people can make custom plugins to add new tabs to the instructor dashboard without having to fork and modify this MFE?

I tried this approach of having a slot per tab, but it seems I can't use Tab component through a slot config, i receive this error msg The Tab component is not meant to be rendered! It's an abstract component that is only valid as a direct Child of the Tabs Component. I guess another approach could be gather the info from the "extended tabs" from slots we want to add, have a context and add it in the main tabs component to render inside that each Tab, but seems overcomplicating this and kind of losing the purpose of having slots 😅 or do you know any workaround for this?
cc. @brian-smith-tcril @bradenmacdonald

brian-smith-tcril · 2025-10-31T18:26:10Z

I tried this approach of having a slot per tab, but it seems I can't use Tab component through a slot config, i receive this error msg The Tab component is not meant to be rendered! It's an abstract component that is only valid as a direct Child of the Tabs Component.

Could you share the code that led to that situation? My guess is something about how that was configured led to

<Tabs>
  <SomethingThatIsNotTab>
    <Tab />
  </SomethingThatIsNotTab>
</Tabs>

I'm SomethingThatIsNotTab might have been SlotContext.Provider considering what Slot renders

  return (
    <SlotContext.Provider value={{ id, children, ...props }}>
      {layoutElement}
    </SlotContext.Provider>
  );

So I think things to look into are:

Modifying Tabs in Paragon to not just directly pass through react-bootstrap Tabs and better support this
Somehow modifying Slot to support "being" a Tab
Creating a TabSlot component that is just a Tab component with a slot in it

I'm not sure how viable any of these options are, but if I were to do some digging those are the places I'd start looking.

diana-villalvazo-wgu · 2025-10-31T18:40:04Z

Could you share the code that led to that situation? My guess is something about how that was configured led to
<Tabs>
  <SomethingThatIsNotTab>
    <Tab />
  </SomethingThatIsNotTab>
</Tabs>
I'm SomethingThatIsNotTab might have been SlotContext.Provider considering what Slot renders
  return (
    <SlotContext.Provider value={{ id, children, ...props }}>
      {layoutElement}
    </SlotContext.Provider>
  );
So I think things to look into are:

Modifying Tabs in Paragon to not just directly pass through react-bootstrap Tabs and better support this

Somehow modifying Slot to support "being" a Tab

Creating a TabSlot component that is just a Tab component with a slot in it

I'm not sure how viable any of these options are, but if I were to do some digging those are the places I'd start looking.

I tried with this code:
WGU-Open-edX@13ae824

i will look into the suggested options and will try to get a way to support this 🤔

bradenmacdonald

Nice work finding a way to get Tabs working with slots! I think we definitely need to make that easier by improving Paragon's <Tab> component directly.

bradenmacdonald · 2025-11-05T20:38:09Z

src/instructorTabs/app.tsx

+  slotId: `org.openedx.frontend.slot.instructor.tabs.v1`,
+  id: `org.openedx.frontend.widget.instructor.tab.${tab_id}`,
+  op: WidgetOperationTypes.APPEND,
+  element: <TabSlot tab_id={tab_id} title={title} url={url} />,


What prevents us from using element: <Tab eventKey={tab_id} title={title} /> directly? Why do we need to have a "fake" <TabSlot> element and then convert it to <Tab> later ?

I guess nothing prevents it, we need to use TabSlot placeholder because if you use Tab directly the app breaks, due to the error explained in prev comments The Tab component is not meant to be rendered! It's an abstract component that is only valid as a direct Child of the Tabs Component.

src/instructorTabs/InstructorTabs.tsx

brian-smith-tcril

I really like the solution you came up with for using a dummy component to support adding tabs to InstructorTabs!

I left a comment about the naming of the TabSlot component, I'd like to hear your thoughts on that one.

It's also not fully clear what parts of this PR are actually related to instructor tabs. For example, the added test in api.test.ts is just checking for a course_name, which feels completely unrelated to adding the tabs.

It'd be great to split this PR up a bit, I'd be happy to review/land a couple small "add some api tests" PRs so this can be rebased and the diff can be more focused on the changes specific for instructor tabs.

brian-smith-tcril · 2025-11-20T16:58:47Z

src/slots/instructorTabsSlot/TabSlot.tsx

+// Since we are using a slot-based architecture and Paragon is passing Tabs/Tab through
+// We can't have context provider between Tabs and Tab when rendering it should be direct parent/children relation
+
+const TabSlot = (_props: TabProps) => {


The name TabSlot feels a bit confusing to me. To me it implies "a slot that a tab goes in," but the README example has an app.tsx where the element going into the InstructorTabsSlot is a TabSlot.

I don't know what a perfect name for this would be, the first thing that comes to mind would be InstructorTab.

To keep the intention of the component clear, I think it might make sense to combine this file and InstructorTabsSlot.tsx into something like

import { Slot } from '@openedx/frontend-base'; import InstructorTabs, { TabProps } from '../../instructorTabs/InstructorTabs'; export const InstructorTabsSlot = () => ( <Slot id="org.openedx.frontend.slot.instructor.tabs.v1"> <InstructorTabs /> </Slot> ); // This component will be a placeholder/dummy component just to retrieve Tab props // Since we are using a slot-based architecture and Paragon is passing Tabs/Tab through // We can't have context provider between Tabs and Tab when rendering it should be direct parent/children relation export const InstructorTab = (_props: TabProps) => { return null; } export default InstructorTabsSlot;

and then in the README example it could be

import { SlotOperation, WidgetOperationTypes } from '@openedx/frontend-base'; import { InstructorTab } from '../slots/instructorTabsSlot/InstructorTabSlot'; // Tab configuration data const tabData = { tab_id: 'course_info', url: 'course_info', title: 'Course Info' }; // Create slot operations export const tabSlots: SlotOperation[] = [{ slotId: `org.openedx.frontend.slot.instructor.tabs.v1`, id: `org.openedx.frontend.widget.instructor.tab.${tab_id}`, op: WidgetOperationTypes.APPEND, element: <InstructorTab tab_id={tabData.tab_id} title={tabData.title} url={tabData.url} />, }];

Thanks Brian, yes after the changes i forgot to update Read me file, but this sounds great, i updated it! 👍

and regarding the api tests, i just added a couple of tests since i need course info api call to retrieve the tabs response :)

bradenmacdonald · 2025-11-20T18:30:54Z

I'll let @brian-smith-tcril do the remaining review here, but let me know if there was any specific input you wanted from me. Thanks for this :)

brian-smith-tcril · 2025-11-20T19:38:00Z

src/instructorTabs/InstructorTabs.tsx

+  const apiTabs: TabProps[] = courseInfo?.tabs ?? [];
+  const allTabs = [...apiTabs];
+
+  widgetPropsArray.forEach(slotTab => {
+    if (!apiTabs.find(apiTab => apiTab.tab_id === slotTab.tab_id)) {
+      allTabs.push(slotTab);
+    }
+  });


Couple of questions about the logic here.

My understanding of the current logic is that it

Grabs tabs from the API, puts those first in the array

Grabs tabs added to the InstructorTabsSlot

If the id matches a tab the API is already providing, the slot tab is ignored

If the id doesn't match a tab the API is already providing, the slot tab is added to the end of the array

So my open questions are:

How do we want to handle ordering of tabs?

Do we always want the API provided tabs to take priority over slot provided tabs?

right now we are taking the ordering from backend response, based on figma, and as you may know, there are some cases that some tabs will not show up, etc, so we rely on the response since we don't want to cause some coupling in case we change tab name at some point, we discussed on backend PR

And for the second one, i was in doubt on which one should take priority, only reason i keep it this way is because if we override through the widget it will just render tab in different order and tab content will be same that we provide instead of what user may expect, so if its same tab name and they want change tab content, they should do it in routes instead of adding a new slot if that is the need 🤔

I'm not sure I'm following. I'd like to think through a few examples to ensure we're covering use cases properly.

A site operator wants to put a new custom tab first

A site operator wants to put a custom tab somewhere in the middle of the existing tabs

A site operator wants to hide one or more tabs coming from the API

A site operator wants to rename a tab

A site operator wants to reorder the tabs coming from the API

For the cases 1, 2 and 5, are possible if i change this implementation and instead of giving priority to apiTabs we allow to delete and add it from slotTabs if they have same tab_id, for example in slot tabs they can give the same as api tabs but in a different order, and we preserve slot tabs order.

Same for case 4 if we preserve giving priority to slotTabs they can provide same tab_id but change title (that is the displayed name)

For case 3 with current implementation we can't suppress a tab, we should implement a new logic for that

I was thinking doing something like this:

widgetPropsArray.forEach(slotTab => { if (!apiTabs.find(apiTab => apiTab.tab_id === slotTab.tab_id)) { allTabs.push(slotTab); } else { const indexToRemove = allTabs.findIndex(({ tab_id }) => tab_id === slotTab.tab_id); if (indexToRemove !== -1) { allTabs.splice(indexToRemove, 1); } allTabs.push(slotTab); } });

mphilbrick211 · 2025-12-11T01:42:32Z

Bumping this, @brian-smith-tcril

brian-smith-tcril

This isn't a complete review as I'm still thinking through how all of this works together. From what I can tell it's looking pretty great, and the documentation is very nice!

I left a couple minor comments for now, and I'll review more ASAP.

brian-smith-tcril · 2026-01-05T20:11:26Z

src/data/api.test.ts

-  const mockCourseData = { course_name: 'Test Course' };
-  const mockCamelCaseData = { courseName: 'Test Course' };
+  const mockCourseData = { course_name: 'Test Course', tabs: [{ tab_id: 'course_info', title: 'Course Information', url: 'https://test-lms.com/courses/test-course-123/info' }] };
+  const mockCamelCasedCourseData = { courseName: 'Test Course', tabs: [{ tabId: 'course_info', title: 'Course Information', url: 'https://test-lms.com/courses/test-course-123/info' }] };


I'm not sure what patterns we're using in other tests, but it'd be nice to reduce the duplication here a bit as the data we're mocking grows. Maybe something like

const mockCourseData = { course_name: 'Test Course', [...] } const mockCamelCasedCourseData = { courseName: mockCourseData.course_name, [...] }

brian-smith-tcril · 2026-01-05T20:15:50Z

src/data/apiHook.ts

  useQuery({
    queryKey: courseInfoQueryKeys.byCourse(courseId),
    queryFn: () => getCourseInfo(courseId),
+    enabled: !!courseId,


Is checking courseId for truthiness here enough validation? My first impression is that this would lead to enabled being true when we have an invalid (but truthy) courseId.

brian-smith-tcril · 2026-01-05T20:44:39Z

src/routes.tsx

+        path: ':tabId',
+        element: <TabContent />


I'm curious about the motivation behind this change. I don't immediately see a need for these pages to have loaders or actions (and therefore errorElements), but moving this logic to a component feels like "locking in" on that decision.

i remember adding this so we can get tabId from useParams for InstructorTabs component, otherwise i didn't get that info, but if you know a better approach to do this glad to hear about it

brian-smith-tcril

After looking through this again I think I have a fuller sense of how things are working.

I'd love to see some clarifying inline comments in here. I left some comments with examples, but overall it'd be great to have the comments as a place to discuss what the code is doing and why, and the code as a place discuss how it is being done.

The other major open question I have is: how can a site operator add content (read: a new page) to a custom tab? It seems like that functionality isn't implemented yet, and I think the answer to that question will inform how we want to define the slots we're building here.

This is definitely a complex thing to build out. Thank you so much for taking it on and working through this review process with me!

brian-smith-tcril · 2026-01-06T19:09:27Z

src/instructorTabs/InstructorTabs.tsx

+  const apiTabs: TabProps[] = courseInfo?.tabs ?? [];
+  const allTabs = [...apiTabs];
+
+  widgetPropsArray.forEach(slotTab => {


It'd be great to add a clarifying comment here, something like

Suggested change

widgetPropsArray.forEach(slotTab => {

// Tabs added via slot take priority over (read: replace) tabs from the API

// All tabs added via slot are placed at the end of the tabs array

widgetPropsArray.forEach(slotTab => {

brian-smith-tcril · 2026-01-06T19:10:28Z

src/instructorTabs/InstructorTabs.tsx

+
+export interface TabProps {
+  tabId: string,
+  url: string,


Adding a clarifying comment here would be great, something like:

Suggested change

url: string,

url: string, // This is not used by frontend-app-instruct, it is a holdover from how the legacy instructor dash used the API

openedx-webhooks added open-source-contribution PR author is not from Axim or 2U core contributor PR author is a Core Contributor (who may or may not have write access to this repo). labels Oct 9, 2025

openedx-webhooks added this to Contributions Oct 9, 2025

github-project-automation bot moved this to Needs Triage in Contributions Oct 9, 2025

diana-villalvazo-wgu added the mao-onboarding Reviewing this will help onboard devs from an Axim mission-aligned organization (MAO). label Oct 10, 2025

mphilbrick211 moved this from Needs Triage to Waiting on Author in Contributions Oct 15, 2025

diana-villalvazo-wgu force-pushed the 23/instructorTabs branch from 80b8171 to 3cf3870 Compare November 5, 2025 02:47

diana-villalvazo-wgu requested a review from brian-smith-tcril November 5, 2025 02:48

diana-villalvazo-wgu marked this pull request as ready for review November 5, 2025 19:53

bradenmacdonald reviewed Nov 5, 2025

View reviewed changes

diana-villalvazo-wgu force-pushed the 23/instructorTabs branch 3 times, most recently from 1fed473 to 25163bb Compare November 13, 2025 01:11

diana-villalvazo-wgu force-pushed the 23/instructorTabs branch from 25163bb to a5cbad5 Compare November 20, 2025 00:55

diana-villalvazo-wgu requested a review from bradenmacdonald November 20, 2025 01:16

diana-villalvazo-wgu force-pushed the 23/instructorTabs branch from 882acc7 to 4ac0580 Compare November 20, 2025 01:19

diana-villalvazo-wgu moved this from Waiting on Author to Ready for Review in Contributions Nov 20, 2025

brian-smith-tcril reviewed Nov 20, 2025

View reviewed changes

diana-villalvazo-wgu force-pushed the 23/instructorTabs branch from 0f7d18e to c4f6ea2 Compare November 20, 2025 17:32

diana-villalvazo-wgu requested a review from brian-smith-tcril November 20, 2025 17:38

brian-smith-tcril reviewed Nov 20, 2025

View reviewed changes

diana-villalvazo-wgu requested a review from brian-smith-tcril November 20, 2025 19:50

diana-villalvazo-wgu force-pushed the 23/instructorTabs branch 2 times, most recently from d0abfbf to 0008c6f Compare December 5, 2025 14:31

diana-villalvazo-wgu force-pushed the 23/instructorTabs branch from 0008c6f to 84d6171 Compare December 10, 2025 00:35

mphilbrick211 moved this from Ready for Review to In Eng Review in Contributions Dec 11, 2025

brian-smith-tcril reviewed Jan 5, 2026

View reviewed changes

brian-smith-tcril reviewed Jan 6, 2026

View reviewed changes

diana-villalvazo-wgu added 11 commits January 8, 2026 10:23

feat: instructor tabs

bed02db

chore: add instructor tabs slot

6521fab

feat: having a slot per tab

02b87df

fix: useWidgetsById instead of useSlotOperations

a2751cb

fix: useParams instead on complicated logic

69e9481

feat: adding endpoint call

1168171

fix: tab content handler

6e27721

fix: add backend call and support static and dynamic tabs

5d2b035

fix: update read me and rename instructor tab

7edb159

chore: preserve slotTabs info

97e6f59

fix: use camel case response

7b256f4

diana-villalvazo-wgu force-pushed the 23/instructorTabs branch from 84d6171 to 7b256f4 Compare January 8, 2026 16:24

	url: string,
	url: string, // This is not used by frontend-app-instruct, it is a holdover from how the legacy instructor dash used the API

Instructor tabs #32

Are you sure you want to change the base?

Instructor tabs #32

Uh oh!

Conversation

diana-villalvazo-wgu commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Demo

Support Information

Uh oh!

openedx-webhooks commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bradenmacdonald commented Oct 9, 2025

Uh oh!

diana-villalvazo-wgu commented Oct 9, 2025

Uh oh!

bradenmacdonald commented Oct 10, 2025

Uh oh!

diana-villalvazo-wgu commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brian-smith-tcril commented Oct 31, 2025

Uh oh!

diana-villalvazo-wgu commented Oct 31, 2025

Uh oh!

bradenmacdonald left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diana-villalvazo-wgu Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brian-smith-tcril left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradenmacdonald commented Nov 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diana-villalvazo-wgu Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diana-villalvazo-wgu Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mphilbrick211 commented Dec 11, 2025

Uh oh!

brian-smith-tcril left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diana-villalvazo-wgu commented Oct 9, 2025 •

edited

Loading

openedx-webhooks commented Oct 9, 2025 •

edited

Loading

codecov bot commented Oct 9, 2025 •

edited

Loading

diana-villalvazo-wgu commented Oct 31, 2025 •

edited

Loading

diana-villalvazo-wgu Nov 11, 2025 •

edited

Loading

diana-villalvazo-wgu Nov 20, 2025 •

edited

Loading

diana-villalvazo-wgu Nov 21, 2025 •

edited

Loading