-
Notifications
You must be signed in to change notification settings - Fork 211
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!: organize plugin slots as components, add footer slot #1381
Conversation
2f9e410
to
30832c7
Compare
@leangseu-edx, mind taking a look? We're trying to standardize how slots (and ids) are exposed. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1381 +/- ##
==========================================
+ Coverage 88.74% 88.75% +0.01%
==========================================
Files 303 305 +2
Lines 5224 5231 +7
Branches 1297 1297
==========================================
+ Hits 4636 4643 +7
Misses 572 572
Partials 16 16 ☔ View full report in Codecov by Sentry. |
61d479b
to
4577218
Compare
4577218
to
f662654
Compare
|
||
## Description | ||
|
||
This slot is used for adding content after the Sequence content section. |
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.
@MaxFrank13 I'm not 💯 on the wording I have here, if you have any suggestions that'd be wonderful!
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.
As someone who doesn't develop much in this MFE, this makes sense to me — which I think is an indication that it's written well 👍
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.
This is great! But as noted below, we've agreed to attempt a refactor of how the footer slot works.
@@ -0,0 +1,48 @@ | |||
# Footer 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.
Just a note for the purpose of transparency: we've decided to attempt to move the Footer Slot to frontend-component-footer
itself.
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.
f662654
to
98fdde1
Compare
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.
Looks great!
But since this is a breaking change, I'm going to hold off a bit until @leangseu-edx approves explicitly.
@brian-smith-tcril, FYI, the following new slot is likely to merge pretty soon, and we'll probably want to include it here in the refactor: |
0e0e67b
to
df9e852
Compare
BREAKING CHANGE: slot ids have been changed for consistency * `sequence_container_plugin` -> `sequence_container_slot` * `unit_title_plugin` -> `unit_title_slot`
df9e852
to
5509327
Compare
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.
Looks great, thanks!
…1381) BREAKING CHANGE: slot ids have been changed for consistency * `sequence_container_plugin` -> `sequence_container_slot` * `unit_title_plugin` -> `unit_title_slot`
BREAKING CHANGE: slot ids have been changed for consistency
sequence_container_plugin
->sequence_container_slot
unit_title_plugin
->unit_title_slot