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][Plugin] Course outline sidebar #1344

Closed

Conversation

ihor-romaniuk
Copy link
Contributor

Settings

EDX_PLATFORM_REPOSITORY: https://github.com/raccoongang/edx-platform.git
EDX_PLATFORM_VERSION: ts-develop

Description

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

Note: Initial solution without using Frontend Plugin Framework #1342

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.

ihor-romaniuk and others added 12 commits April 1, 2024 17:43
* feat: [AXIMST-572] create course-outline sidebar

* fix: [AXIMST-572] after review
* feat: [AXIMST-617] display section and sequence sidebar level

* fix: after review

* fix: after review
* feat: [AXIMST-590] display units on sidebar, two tier layout

* fix: expand prop-types

* fix: after review
* feat: [AXIMST-578] display outline sidebar depends on feature flag

* fix: after review
* feat: [AXIMST-635] display discussions sidebar by waffle flag

* fix: after demo

* fix: after review

* fix: upstream failed tests
* feat: [AXIMST-641] display special exam label and unit lock icon

* fix: after backend changes

* fix: error with entrance exam

* fix: after review
* feat: make sidebar fixed on scroll and add tests

* fix: extend tests

* fix: tests

* fix: codecov

* fix: after review
…plete subsection (#28)

* fix: [AXIMST-748] update outline sidebar for locked subsection on complete subsection

* fix: tests

* fix: after review
@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Apr 4, 2024
@openedx-webhooks
Copy link

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.

This is currently a draft pull request. When it is ready for our review and all tests are green, click "Ready for Review", or remove "WIP" from the title, as appropriate.

@ihor-romaniuk ihor-romaniuk force-pushed the romaniuk/sidebar-main-plugin branch 2 times, most recently from 72f08f9 to e0b25b1 Compare April 8, 2024 09:50
Comment on lines +24 to +30
export const COURSE_OUTLINE_SIDEBAR_ID = 'COURSE_OUTLINE_SIDEBAR';

export const extendSidebars = (key, value) => {
SIDEBARS[key] = value;
};

export const checkIsSidebarAvailable = (id) => Object.keys(SIDEBARS).includes(id);
Copy link
Contributor Author

@ihor-romaniuk ihor-romaniuk Apr 8, 2024

Choose a reason for hiding this comment

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

Through the close interaction of the Outline Sidebar plugin's display with the application's functionality display (sidebar display order, breadcrumb display, etc.), we should have a way to check for an active Outline Sidebar plugin.

Perhaps it should be reworked and placed in a global state and have a list of connected plugins.

Comment on lines +416 to +417
// Import plugin-specific sass files
@import "../plugins/index.scss";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We needed to be able to include style files from plugins with the ability to use the Paragon system, so we decided to add one entry point for all plugin styles.

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 for taking on this work! My first big request for a change, however, is the following:

Any feature that should be on by default - which is the case with this sidebar - should not be a plugin. It can be implemented as default content inside a plugin slot, but users should not have to change their configuration to get the corresponding functionality.

In other words, I would like to see all components and corresponding data functions moved out of plugins, but with one caveat: we should create an example plugin that disables the functionality being introduced, and document how to use it. This means we'll also have to test that this "disable plugin" actually works.

@ihor-romaniuk
Copy link
Contributor Author

@arbrandes I created a new PR #1349, which will wrap insertion points of triggers for calling the navigation sidebar, added sidebar accessibility checks to hide breadcrumbs when a sidebar is present, and added an example of a standard configuration in env.config.jsx.

@itsjeyd itsjeyd added the product review PR requires product review before merging label Apr 22, 2024
@itsjeyd
Copy link

itsjeyd commented Apr 26, 2024

@ihor-romaniuk

I created a new PR #1349, ...

Does that PR replace this one? Asking because I'd like to understand if we need to keep this PR open.

@itsjeyd itsjeyd added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Apr 26, 2024
@itsjeyd
Copy link

itsjeyd commented Apr 26, 2024

OSPR management note: This is part of a set of PRs for implementing Phase 1 changes for the FC-0056 project. We can consider it approved from the product perspective.

CC @GlugovGrGlib

@openedx-webhooks
Copy link

@ihor-romaniuk Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

@ihor-romaniuk
Copy link
Contributor Author

The main PR about the course outline sidebar functionality #1349

@itsjeyd itsjeyd removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U product review PR requires product review before merging
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants