-
Notifications
You must be signed in to change notification settings - Fork 106
feat: Add plugins slots for sidebars and application #1752
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
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. |
266e45a
to
4b483c5
Compare
@xitij2000 please rebase your PR on master :) |
4f28c86
to
19d60fa
Compare
@farhaanbukhsh Done. Note: currently the plugin slots are not extracted into the plugin-slots folder since I want to get feedback abotu the location names etc first. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1752 +/- ##
==========================================
+ Coverage 93.50% 93.53% +0.02%
==========================================
Files 1128 1132 +4
Lines 22925 23043 +118
Branches 4953 4978 +25
==========================================
+ Hits 21436 21553 +117
- Misses 1413 1414 +1
Partials 76 76 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@xitij2000 I think the names LGTM and we can extract this to the plugin slot :) @brian-smith-tcril Do you think the slot names are okay here? |
In the future we'll want to have names that follow the format described here. For now these names seem mostly reasonable, but I have a few thoughts:
As for |
2f055db
to
bebfabd
Compare
@brian-smith-tcril Thanks for the feedback! For consistency, should I also rename I've put the reasoning (and alternatives) for the |
That sounds great!
I think the full MFE wrapper slot pattern should be a bigger discussion. Would you mind splitting this PR up so we can land the outline sidebar and unit sidebar slots first? I don't want the discussion to block the uncontroversial additions. |
Sure, I will do that. |
6db68df
to
38caa60
Compare
Add slots for the sidebar on the course outline page and the unit page.
38caa60
to
633fb23
Compare
I've created a separate PR for the full app slot: #1799 |
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.
Overall this is looking good! I left a couple little comments about names and docs. Once those are addressed this should be good to go!
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 screenshot names might have gotten mixed up. This is showing a border around the entire page.
@xitij2000 it looks like this is failing lint, could you run |
Odd, I did run that before committing, but it seem it couldn't fix this. I've fixed it manually and pushed. |
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.
Super close to ready to land. Just the one issue with the image.
Description
This PR adds three slots to the authoring app:
Supporting information
NA
Testing instructions
NA
Other information