-
Notifications
You must be signed in to change notification settings - Fork 290
feat: Add slots to add tab links and add mechanism for plugin routes #1645
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
base: master
Are you sure you want to change the base?
Conversation
Thanks for the pull request, @xitij2000! This repository is currently maintained by 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 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. 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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1645 +/- ##
==========================================
+ Coverage 90.83% 90.87% +0.03%
==========================================
Files 344 347 +3
Lines 5794 5816 +22
Branches 1374 1373 -1
==========================================
+ Hits 5263 5285 +22
Misses 512 512
Partials 19 19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
83693c3
to
b9df318
Compare
7245d96
to
e040882
Compare
7affcf0
to
fd9df35
Compare
@bradenmacdonald Would love to get your thoughts on the approach used in this PR. It's doing a few non-standard things. We wanted to add a new course tab, for which we need two things.
To add a new page that feels like part of the platform we need some way to extend the routes to allow creating a new page. I tried making the routes pluggable, but the router refuses to work with components that aren't a Route. Which led to the approach used here, which is to add a config value for defining new routes, each route gets its own slot so you can create a new page there. |
@xitij2000 I don't think plugins can add This is because:
See https://github.com/openedx/frontend-app-authoring/blob/aeefcc639f430c4769b0903cad0da7977cb40cf3/src/pages-and-resources/PagesAndResources.jsx#L88-L93 for an example of this. It's not wrapped in a plugin slot, but it would work just the same way if it were. Also, I think we want to move away from a very config-heavy plugin approach toward something where plugins "register themselves" and require minimal config other than installation, so I think this sort of approach (adding |
Hey @xitij2000, are you still planning to continue working on this PR? |
@itsjeyd I'm looking into the budget situation for this, will update once that is clear. |
d897611
to
3f48bac
Compare
@bradenmacdonald I've switched to the mechanism you recommended. One issue is that now I can no longer have a route like |
3f48bac
to
0281e1a
Compare
@xitij2000 Sorry for the slow reply. If the slot is named "Course Page Slot" then maybe you should mount the slot as a catch-all for |
I tried this and unfortunately it is simply not working. It keeps getting redirected to the home page. I think this is because there is already a match for /course/:courseId/:sequenceId which is also catchall. The only way out I think would be to have |
@xitij2000 Ah, I see. Well thanks for trying. What do you think makes the most sense then? I kinda like the idea of having a a slot under Though maybe we should also ask @arbrandes if there's a more official way for plugins to add new URLs, especially in frontend-base world. |
That is a good point. I think there is a better way with frontend-base which AFAIK would work across all MFEs. From what I remember with frontend-base you can simply extend the list of routes via frontend-template-site. So at that point of time this work might not be needed at all. Is my understandin correct @arbrandes? If so we can pause this work and just temporarily maintain the drift on the client side till the better approach from frontend-base is implemented. |
That is correct. Any plugin (any "app") will be able to easily extend routes. You can see how it works in the "slot showcase" plugin/app (usable by running |
@arbrandes Thanks for clarifying! @bradenmacdonald I think given the above it might make sense to refactor this PR to only contain the changes relevant to adding a new tab and remove the code related to custom routes since that approach will change with the move to frontend-base. For now we can keep this bit of code drift in the client branch. CC: @pkulkark |
@xitij2000 OK, makes sense. |
0281e1a
to
dc99d92
Compare
@bradenmacdonald I've removed all the routes-related code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xitij2000 This looks good to me, though @farhaanbukhsh is the maintainer now and @arbrandes knows more about plugin slots and the future direction of this MFE. Do either of you guys want to look at this or approve it for us?
<a | ||
className={classNames('nav-item flex-shrink-0 nav-link')} | ||
href="#" | ||
> | ||
Custom Tab | ||
</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the example use CourseTabLink
or is that not part of the plugin API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was a last minute refactoring so forgot to update the docs. It makes sense to have that here. Will update.
468a280
to
e10f3b0
Compare
Adds new slot that allow adding new links to course tabs.
e10f3b0
to
06aa67f
Compare
Description
This PR adds mechanisms to add course tabs using a frontend plugin.