feat: remove waffle flags for managing course outline sidebar #1713
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @ihor-romaniuk! 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. |
ihor-romaniuk
force-pushed
the
romaniuk/remove-waffle-flags-for-managing-outline-sidebar
branch
from
f7a40b1 to
2fb7ca8
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1713 +/- ##
==========================================
- Coverage 90.45% 90.33% -0.12%
==========================================
Files 344 344
Lines 5802 5793 -9
Branches 1395 1394 -1
==========================================
- Hits 5248 5233 -15
- Misses 535 541 +6
Partials 19 19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ihor-romaniuk
added
the
create-sandbox
|
I haven't taken a full look at this yet, but my first impressions are that it is looking pretty good. Would you be able to break off the part adding the |
|
@brian-smith-tcril I prepared new pull request with moving the |
|
Sandbox deployment successful 🚀 |
ihor-romaniuk
force-pushed
the
romaniuk/remove-waffle-flags-for-managing-outline-sidebar
branch
2 times, most recently
from
ac4b03d to
c18958a
Compare
There was a problem hiding this comment.
Unless I’m mistaken, this is a removal PR for a DEPR that hasn’t made it to the Breaking Changes Unblocked state. I think the DEPR ticket may need a coordinator from the DEPR WG assigned to help ensure the changes move forward without hurting deployments, especially early deployers. Thank you.
I’m temporarily marking with Request Changes to ensure this doesn’t merge yet.
|
I've updated the DEPR and am seeking feedback on timing in https://discuss.openedx.org/t/deprecation-ability-to-modify-learning-mfe-sidebar-behavior-with-waffle-flags/15119/4?u=feanil Given that the replacement has existed for some time and there were no objections, I'm proposing we land this by next week, please chime in on the DEPR ticket or the discuss thread about concerns with that timing. @ihor-romaniuk thanks for the PR, sorry about the DEPR process churn, we're in the middle of changing that process so there is a bit of adapting we have to do. |
|
Sandbox deployment failed 💥 |
ihor-romaniuk
added
create-sandbox
|
Sandbox deployment failed 💥 |
|
Sandbox deployment successful 🚀 |
| @@ -170,7 +169,7 @@ const Sequence = ({ | |||
| /> | |||
| <CourseOutlineSidebarSlot /> | |||
| <div className="sequence w-100"> | |||
| {!isEnabledOutlineSidebar && ( | |||
| {!getConfig().ENABLE_SEQUENCE_NAVIGATION && ( | |||
There was a problem hiding this comment.
- It seems like
ENABLE_SEQUENCE_NAVIGATIONis new, was a specific decision made to usegetConfig()for this as opposed to relying on FPF slots? - The logic here feels backwards, as it stands it seems
SequenceNavigationis rendered whengetConfig().ENABLE_SEQUENCE_NAVIGATIONis falsy.
There was a problem hiding this comment.
- You are right. The
ENABLE_SEQUENCE_NAVIGATIONconfig is new, and its main purpose is to allow this functionality to remain disabled by default, since as far as I know, we cannot keep a plugin slot disabled by default. - That was my mistake, and it has been fixed.
There was a problem hiding this comment.
The goal here would not be to enable/disable the slot - the slot should always be rendered. The goal is to render nothing in the slot by default.
The way I would handle this using only FPF slots would be to:
- Keep the
SequenceNavigationin the codebase, but no longer have it as the default content of theSequenceNavigationSlot. - Update the
SequenceNavigationSlotREADME to include an exampleenv.config.jsxshowing how to use theSequenceNavigationcomponent in theSequenceNavigationSlot
There was a problem hiding this comment.
I have updated pull request with:
- set default (empty) state to
SequenceNavigationSlot - add an example of usage
SequenceNavigationSlotwith a defaultSequenceNavigationcomponent - update tests (code coverage is a bit lower since we removed the
SequenceNavigationcomponent from the code and I don't think it needs to be tested separately since it's an option we don't use by default)
|
Sandbox deployment successful 🚀 |
ihor-romaniuk
force-pushed
the
romaniuk/remove-waffle-flags-for-managing-outline-sidebar
branch
from
a8c907a to
1a706f5
Compare
ihor-romaniuk
force-pushed
the
romaniuk/remove-waffle-flags-for-managing-outline-sidebar
branch
from
1a706f5 to
9d06665
Compare
itsjeyd
added
the
waiting on author
|
Sandbox deployment successful 🚀 |
ihor-romaniuk
force-pushed
the
romaniuk/remove-waffle-flags-for-managing-outline-sidebar
branch
from
1121eed to
2e8e6dc
Compare
|
Sandbox deployment failed 💥 |
|
Sandbox deployment failed 💥 |
|
@ihor-romaniuk I discussed the versioning question with @arbrandes and some other Axim engineers and we decided that this does not need a version bump From the versioning docs:
This is a bit of an odd situation because the versioning docs also say
This led us to discuss needing a process for communicating default content changes outside of slot versioning. I have created an issue on the FPF repo to discuss the best way to turn this into an ADR. |
|
Sandbox deployment successful 🚀 |
|
@brian-smith-tcril Should we revert our changes regarding bumping plugin slot version or we should wait for result of the discussion "Docs: Create ADR for default content versioning" |
@ihor-romaniuk for this PR just reverting the version bump should be fine. I do want to figure out a default content versioning plan but that shouldn't block this work. |
|
@brian-smith-tcril I’ve reverted the recent changes related to the plugin slot version bump. The PR is now ready for review. |
|
Sandbox deployment failed 💥 |
ihor-romaniuk
added
create-sandbox
|
Sandbox deployment successful 🚀 |
|
@ihor-romaniuk it looks like code coverage for this change is quite low. I see that some tests have been removed. Could you look into updating some of those tests instead of removing them? |
ihor-romaniuk
force-pushed
the
romaniuk/remove-waffle-flags-for-managing-outline-sidebar
branch
5 times, most recently
from
06c4cb1 to
60ed4d3
Compare
ihor-romaniuk
force-pushed
the
romaniuk/remove-waffle-flags-for-managing-outline-sidebar
branch
from
60ed4d3 to
ead604f
Compare
@brian-smith-tcril I’ve updated the tests and excluded unused methods from the Codecov coverage report, as these handlers are passed solely to support the props required by the plugin slot, though they are not used by default. |
|
Sandbox deployment successful 🚀 |
DEPR: openedx/public-engineering#316 (comment)
Description
This PR removes support for the deprecated waffle flags:
courseware.enable_navigation_sidebarcourseware.always_open_auxiliary_sidebarThese flags have been replaced by PluginSlots introduced in PR #1543, available starting from the Teak release.
The default behavior is now:
All customization should now be handled via the Frontend Plugin Framework using
PluginSlot.This change simplifies the sidebar logic and reduces technical debt.
Testing Instructions
PluginSlotstill function correctly.Changelog
courseware.enable_navigation_sidebar,courseware.always_open_auxiliary_sidebar.