Skip to content
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

[FC-0056] Course outline sidebar #1375

Merged

Conversation

ihor-romaniuk
Copy link
Contributor

@ihor-romaniuk ihor-romaniuk commented Apr 30, 2024

Settings

EDX_PLATFORM_REPOSITORY: https://github.com/raccoongang/edx-platform.git
EDX_PLATFORM_VERSION: NiedielnitsevIvan/FC-0056/feature/waffle-flag-for-displaying-discussions-tab

Description

This pull request adds an important feature to our platform: displaying a navigation sidebar within a given course.

Interaction with feature flag courseware.show_default_right_sidebar has been added to control the default display of the discussion sidebar and courseware.disable_navigation_sidebar to control of enabling the course outline sidebar.

Discussions or Notifications sidebar shouldn't be opened on Learning MFE by default. If waffle flag enabled - Discussions always opens on the pages with discussions, if user is in Audit and course has verified mode - show Notifications.

Depends on BE PRs

Design

https://www.figma.com/file/gew5tORDX4Q7wxOS8vjqZu/side-nav-OEX?type=design&node-id=318-3234&mode=design&t=rBe1ToNYP8RY6QOp-0

image

Testing instructions

  1. Run master devstack.
  2. Start platform make dev.up.lms+cms+frontend-app-course-authoring and make checkout on this branch.
  3. Go to Course Unit page from the Course Outline page.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Apr 30, 2024
@openedx-webhooks
Copy link

openedx-webhooks commented Apr 30, 2024

Thanks for the pull request, @ihor-romaniuk! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@ihor-romaniuk ihor-romaniuk force-pushed the romaniuk/course-outline-sidebar branch from 39a3d8e to 27e325f Compare April 30, 2024 08:49
@ihor-romaniuk ihor-romaniuk changed the title feat: [FC-0056] create course outline sidebar [FC-0056] Course outline sidebar Apr 30, 2024
Copy link

codecov bot commented Apr 30, 2024

Codecov Report

Attention: Patch coverage is 97.89474% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 88.72%. Comparing base (cf3a91d) to head (24db8c1).
Report is 2 commits behind head on master.

Files Patch % Lines
...sidebars/course-outline/components/SidebarUnit.jsx 81.81% 2 Missing ⚠️
src/courseware/data/utils.js 96.00% 2 Missing ⚠️
...urseware/course/sidebar/SidebarContextProvider.jsx 93.75% 1 Missing ⚠️
src/courseware/data/slice.js 96.96% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1375      +/-   ##
==========================================
+ Coverage   88.30%   88.72%   +0.42%     
==========================================
  Files         292      302      +10     
  Lines        5002     5217     +215     
  Branches     1267     1295      +28     
==========================================
+ Hits         4417     4629     +212     
- Misses        569      572       +3     
  Partials       16       16              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ihor-romaniuk ihor-romaniuk added the create-sandbox open-craft-grove should create a sandbox environment from this PR label Apr 30, 2024
@open-craft-grove
Copy link

Sandbox deployment failed 💥
Please check the settings and requirements.
Retry deployment by pushing a new commit or updating the requirements/settings in the pull request's description.
📜 Failure Logs
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@brian-smith-tcril
Copy link
Contributor

I was able to get this working in tutor locally by merging openedx/edx-platform#34650 into a new branch I made off of latest edx-platform master https://github.com/brian-smith-tcril/edx-platform/tree/sidebar-early-merge

I ran

tutor images build openedx-dev --build-arg EDX_PLATFORM_REPOSITORY=https://github.com/brian-smith-tcril/edx-platform.git --build-arg EDX_PLATFORM_VERSION=sidebar-early-merge

and used this plugin

from tutormfe.hooks import MFE_APPS

@MFE_APPS.add()
def _add_my_mfe(mfes):
    mfes["learning"] = {
        "repository": "https://github.com/raccoongang/frontend-app-learning.git",
        "port": 2000,
        "version": "romaniuk/course-outline-sidebar",
    }
    return mfes

Since the ts-develop branch is ~150 commits behind upstream, it is not working with latest tutor nightly (I believe this is because of the move from node 16 to node 18)

I believe if you rebase openedx/edx-platform#34650 on latest upstream master and point to that branch instead of ts-develop the sandbox should deploy properly.

@arbrandes
Copy link

Until the required backend PR is merged, I'm moving this to Draft status.

@arbrandes arbrandes marked this pull request as draft May 1, 2024 15:16
@ihor-romaniuk
Copy link
Contributor Author

ihor-romaniuk commented May 2, 2024

@brian-smith-tcril Thanks for notice that.
I changed the deploy branch to NiedielnitsevIvan/FC-0056/feature/waffle-flag-for-displaying-discussions-tab which should be up-to-date and include all required backend changes.

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@ihor-romaniuk ihor-romaniuk force-pushed the romaniuk/course-outline-sidebar branch from 27e325f to 47216be Compare May 2, 2024 15:51
@ihor-romaniuk
Copy link
Contributor Author

Update branch with added a few UI enhancements:

  • Highlight the active section
  • Highlight the active subsections when it collapsed
  • Make unit row clickable instead of just unit name

@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

@ihor-romaniuk
Copy link
Contributor Author

Update branch: made a refactoring according to changes in backend endpoints openedx/edx-platform#34650

@ihor-romaniuk ihor-romaniuk marked this pull request as ready for review May 7, 2024 07:50
@open-craft-grove
Copy link

Sandbox deployment successful 🚀
🎓 LMS
📝 Studio
ℹ️ Grove Config, Tutor Config, Tutor Requirements

Copy link

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I find this implementation is easier to reason about than the previous one. Thank you! It will make it easier for us to revisit this later when we attempt to implement the same functionality via PluginSlots.

Oh, and I tested it against a deploy of master via Tutor nightly, and it seems to work well.

I do have a couple of nits though, if you don't mind.

Comment on lines 97 to 100
// const { container } = render(
// <Sequence {...mockData} {...{ sequenceId: sequenceBlocks[0].id }} />,
// { store: testStore, wrapWithRouter: true },
// );

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind removing the commented out code? Unless it's here for documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I remove it

@@ -134,9 +156,10 @@ describe('Sequence', () => {
});

it('handles loading unit', async () => {
render(<Sequence {...mockData} />, { wrapWithRouter: true });
// render(<Sequence {...mockData} />, { wrapWithRouter: true });

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another commented line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also delete

const query = new URLSearchParams(window.location.search);
const initialSidebar = (shouldDisplaySidebarOpen || query.get('sidebar') === 'true') ? SIDEBARS.DISCUSSIONS.ID : null;
const isDefaultDisplayRightSidebar = useSelector(getCoursewareOutlineSidebarSettings).alwaysOpenAuxiliarySidebar;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const isDefaultDisplayRightSidebar = useSelector(getCoursewareOutlineSidebarSettings).alwaysOpenAuxiliarySidebar;
const alwaysOpenAuxiliarySidebar = useSelector(getCoursewareOutlineSidebarSettings).alwaysOpenAuxiliarySidebar;

Do you mind refactoring so that we don't have a mismatch between the waffle flag name and the name we use here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, refactored. Thanks.

@ihor-romaniuk ihor-romaniuk force-pushed the romaniuk/course-outline-sidebar branch from 9edafba to 24db8c1 Compare May 7, 2024 15:51
Copy link

@arbrandes arbrandes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Approved!

@arbrandes arbrandes merged commit 1c3610e into openedx:master May 7, 2024
7 checks passed
@openedx-webhooks
Copy link

@ihor-romaniuk 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@GlugovGrGlib
Copy link
Member

This PR is the part of - openedx/platform-roadmap#329

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
create-sandbox open-craft-grove should create a sandbox environment from this PR open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants