-
Notifications
You must be signed in to change notification settings - Fork 182
feat: adds slots for in-context metrics in studio outline and unit views #1725
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
feat: adds slots for in-context metrics in studio outline and unit views #1725
Conversation
|
Thanks for the pull request, @tecoholic! 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. DetailsWhere 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. |
plugins/aspects/README.md
Outdated
| @@ -0,0 +1,22 @@ | |||
| # Aspects Plugins | |||
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.
Our hope here was that this plugin wouldn't need to reside here, but rather would be able to live in one of the Aspects repos (platform-plugin-aspects or tutor-contrib-aspects). Is that possible to do?
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.
I agree. I wasn't sure where to put this as well, I was hoping @farhaanbukhsh would give me some ideas during our internal review :)
On the tech side, as long as can install it as an NPM package any place should be good. The main side-effect I can think of would be to publish yet another NPM package.
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.
Thank you @bmtcril for the review here, when I went through both repos:
- https://github.com/openedx/platform-plugin-aspects
- https://github.com/openedx/tutor-contrib-aspects/
It made more sense to me to move the plugins under tutor-contrib-aspect since it makes it a whole package of aspect configuration that we can ship and configure. Just like we have clickhouse, Ralph, Vector etc. we also ship a presentation layer(optional to use).
This also opens up the possibility of adding more such plugins there.
I went against the platform plugin aspect because these plugins installed there will make it a very confusing codebase. It will be good to keep all the customization in the tutor-plugin and the more concrete parts in the platform-plugin.
What do you think @tecoholic?
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.
In a discussion yesterday, @bmtcril pointed out the segregation in responsibilities between two repos. Where anything related to LMS/CMS belongs to the platform-plugin-aspect while tutor-contrib-aspects will have all the configurations.
@tecoholic any thoughts?
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.
@farhaanbukhsh The separation of concerns sounds sensible to me.
@bmtcril Practically what would be appropriate in the case? Personally, I think publishing an NPM package like @openedx/frontend-plugin-aspects could be complementary to platform-plugin-aspects. It might mean it's own repo .Or maybe it shares a directory in "platform-plugin-aspects" repo? I am not clear on the logistics of this. Kindly advise. I will move the plugin code accordingly.
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.
@tecoholic @farhaanbukhsh I think what we've come up with on the Axim side is that this should probably live in its own repo (frontend-plugin-aspects) until we have a better way of combining front-end, back-end, and tutor plugins in one place. If that's good for you all I can create the new repo for you in the openedx org and make you the maintainers of it through the support portion of this contract.
That way we don't have to deal with moving it later and setting up all of the automation, and I believe we can just grant you the permissions as the creators of the plugin (instead of going through the CC process).
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.
@bmtcril Great. Can you kindly create the repo and provide access to @farhaanbukhsh. As he is a CC, it should be fairly straightforward than my account without CC rights. He would be the primary reviewer for these changes from OpenCraft's side anyway.
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.
@bmtcril gentle ping on creating the repo 😇
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.
I'm working on it now, it's going to start as a clone of frontend-template-application, but I know that won't be a perfect fit so feel free to adjust as necessary.
brian-smith-tcril
left a comment
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.
Context: I reviewed the plugin slots but not the plugins.
Overall these are looking really good! I'm super happy to see the plugin slots as components pattern being used so nicely!
I left a few comments in there. A couple of them are just little docs suggestions, and the other couple are some open questions about slot naming and flexibility.
| <CourseUnitAnalyticsSlot | ||
| unitTitle={unitTitle} | ||
| isUnitVerticalType={unitCategory === COURSE_BLOCK_NAMES.vertical.id} | ||
| courseVerticalChildren={courseVerticalChildren} | ||
| /> |
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.
Open questions:
- Do we want to allow site operators to customize this beyond adding things at the end? If so, then it would make sense to have this slot wrap the existing buttons instead.
- This slot is not analytics specific, any ideas for a more generic name for this slot?
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.
@brian-smith-tcril Very valid comments.
- Wrapping the full header navigation is an interesting idea. I think it might offer more flexibility in how this slot is used. I am onboard for this idea. @farhaanbukhsh what are your thoughts.
- This was actually something I was planning to do. This should be something generic like
CourseUnitHeaderActionsSlotandCourseOutlineHeaderActionsslot. If there are better suggestions, kindly propose.
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.
CourseUnitHeaderActionsSlot and CourseOutlineHeaderActionsSlot sound great to me!
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.
Do we have multiple actions associated with them?
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.
I think the idea is that the slot could wrap the current "actions" ("New section", "Reindex", "Collapse all", and "View live" in the Course Outline), and allow people to insert/modify them.
If we decide against wrapping the existing actions, and instead continue with slots with no default content, I think something like CourseOutlineHeaderAdditionalActionsSlot might work.
As for singular Action vs plural Actions - there is nothing stopping a site operator from putting multiple things into the slot.
const MyButtons = () => (
<>
<Button>foo</Button>
<Button>bar</Button>
</>
);
const config = {
pluginSlots: {
course_outline_analytics_slot: {
keepDefault: true,
plugins: [
{
op: PLUGIN_OPERATIONS.Insert,
widget: {
id: 'my-extra-button',
priority: 60,
type: DIRECT_PLUGIN,
RenderWidget: MyButtons,
},
},
]
}
},
}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.
@brian-smith-tcril Awesome then it makes sense to have CourseUnitHeaderActionsSlot and CourseOutlineHeaderActionsSlot
| {intl.formatMessage(messages.viewLiveButton)} | ||
| </Button> | ||
| </OverlayTrigger> | ||
| <CourseOutlineAnalyticsSlot hasSections={hasSections} sections={sections} /> |
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.
Open questions:
- Do we want to allow site operators to customize this beyond adding things at the end? If so, then it would make sense to have this slot wrap the existing buttons instead.
- This slot is not analytics specific, any ideas for a more generic name for this slot?
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.
Replied above
|
@brian-smith-tcril Thank you for your wonderful suggestions and comments. I have applied the suggestions and responded to your comments. I am planning on updating the PR after Farhaan's review. |
plugins/aspects/README.md
Outdated
| @@ -0,0 +1,22 @@ | |||
| # Aspects Plugins | |||
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.
Thank you @bmtcril for the review here, when I went through both repos:
- https://github.com/openedx/platform-plugin-aspects
- https://github.com/openedx/tutor-contrib-aspects/
It made more sense to me to move the plugins under tutor-contrib-aspect since it makes it a whole package of aspect configuration that we can ship and configure. Just like we have clickhouse, Ralph, Vector etc. we also ship a presentation layer(optional to use).
This also opens up the possibility of adding more such plugins there.
I went against the platform plugin aspect because these plugins installed there will make it a very confusing codebase. It will be good to keep all the customization in the tutor-plugin and the more concrete parts in the platform-plugin.
What do you think @tecoholic?
| <CourseUnitAnalyticsSlot | ||
| unitTitle={unitTitle} | ||
| isUnitVerticalType={unitCategory === COURSE_BLOCK_NAMES.vertical.id} | ||
| courseVerticalChildren={courseVerticalChildren} | ||
| /> |
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.
Do we have multiple actions associated with them?
| } = headerNavigationsActions; | ||
|
|
||
| return ( | ||
| <nav className="header-navigations ml-auto"> |
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.
Note: This wrapper wasn't needed as the SubHeader component already provides the necessary styling for the headerActions property.
97c459c to
3e77134
Compare
farhaanbukhsh
left a comment
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.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1725 +/- ##
=======================================
Coverage 93.45% 93.46%
=======================================
Files 1120 1124 +4
Lines 22730 22746 +16
Branches 4916 4824 -92
=======================================
+ Hits 21243 21259 +16
Misses 1419 1419
Partials 68 68 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
farhaanbukhsh
left a comment
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.
Great work the code looks beautiful and is working as expected. While I was going through the code and Figma design I noticed that on the unit page also we would need one more slot.
After this change the PR will be complete, sorry for the nitpicking here @tecoholic
|
@farhaanbukhsh Thanks for reviewing this once more and great catch on the missing slot in unit page. Frankly, I am the only who should be sorry for making you go through so many review rounds and missing the details. I will add the extra slot and ping you. One point I forgot to mention, is that there are no tests for the slots in the Unit Card and Subsection card, because there is no functional code to generate test cases. |
|
@farhaanbukhsh I think that slot cannot be implemented at the moment. The content blocks are all rendered by the legacy Studio as an Iframe. The MFE won't be able to provide slots to add buttons in them. Also, this page is currently considered experimental and has to be enabled using a Course Level Waffle Flag. So, this might be possible only when we have these elements implemented in the MFE. |
farhaanbukhsh
left a comment
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.
👍
- ✅ I tested this on Tutor devstack and all the slots are working as expected
- ✅ I read through the code
- ❌ I checked for accessibility issues
- ✅ Includes documentation
|
@brian-smith-tcril can you have a look once? :) |
|
Overall this is looking great! Would you be able to update the slot |
|
@brian-smith-tcril Absolutely. I have added the screenshots with the example configurations for all the slots and updated their READMEs. |
|
@brian-smith-tcril Please have a look and if it looks good, can we merge this? |
|
Once this is rebased and tests are passing I think it's good to merge! |
This commit introduces the dedicated folder to keep the slots of the MFE
This commit adds the slot and a plugin that can be inserted into the slot. # Conflicts: # src/course-unit/header-navigations/HeaderNavigations.jsx
Co-authored-by: Brian Smith <112954497+brian-smith-tcril@users.noreply.github.com>
Co-authored-by: Brian Smith <112954497+brian-smith-tcril@users.noreply.github.com>
Co-authored-by: Brian Smith <112954497+brian-smith-tcril@users.noreply.github.com>
190afcc to
ed20892
Compare
|
@brian-smith-tcril I have rebased on master and the checks seems to have passed :) |
|
@tecoholic Do you mind squashing few of theses commits? I think bunch of the fixes or should I squash all of them into one? In that case can you give me a informative commit message? |
|
@farhaanbukhsh I thought about it during the rebase. I think it's okay to squash them all into one, as the change is adding slots and nothing more. I don't see a benefit in splitting into multiple commits. |




Description
The PR adds new frontend plugin slots for adding in-context metrics to Studio.
Following the convention established in frontend-app-learner-dashboard,
the new slots are defined in the
src/plugin-slotsdirectory. They are thenimported at the appropriate components as expected in the UI. The components
that would provide the metrics from Superset (Aspects) are added to the
repository as plugins in the
plugins/aspectsdirectory. They will beinserted in the slots by tutor-contrib-aspects.
However, these plugins can be manually inserted into the slots by configuring
them in
env.config.jsxfor dev or testing purposes.Design decisions and their rationales should be documented in the repo (docstring / ADR), per
Useful information to include:
Supporting information
Testing instructions
Setup with tutor-contrib-aspects
Warning
This no longer works as the plugins have been moved out. See below for setting up without tutor-contrib-aspects.
tutor-contrib-aspectsfrom the PR branch at In-context metrics for studio tutor-contrib-aspects#1030tutor config saveto generate the necessary configuration.env.config.jsxinto the repo. This is needed as tutor mounts for MFE repos replace the containerdirectory with your local directory, which results in env.config.jsx getting overwritten or removed.
cp $(tutor config printroot)env/plugins/mfe/build/mfe/env.config.jsx /your/path/to/frontend-app-authoringSetup without tutor-contrib-aspects
env.config.jsxinside the repo. (create the file if needed)Both the scenarios will have a workable
env.config.jsxin your repo and ready to go. Now,build the authoring-dev container and start it.
tutor images build authoring-dev,tutor dev stop authroing && tutor dev start authoring -dCourse Outline Analytics
The course outline page should now have an "Analytics" button, without any extra steps.
Course Unit Analytics
By default, the Authoring MFE uses legacy Studio UI for the Course Unit page. In order to test the
PR changes, enable the MFE's course unit page.
/admin/waffle_utils/waffleflagcourseoverridemodel/) with:contentstore.new_studio_mfe.use_new_unit_page.env.privatein the MFE repo with the followingNow Authoring MFE should display the course units withing the MFE and the header should have an "Analytics" button.
Other information
Include anything else that will help reviewers and consumers understand the change.