feat: course outline - sections and subsections list #733

navinkarkera · 2023-12-07T07:33:44Z

This PR combines following PRs to create section and subsection list.

See original PR: feat: course outline section list open-craft/frontend-app-authoring#12 for details about section list.
See original PR: feat: subsection list open-craft/frontend-app-authoring#20 for details about subsection list.
See original PR: Implement Section Configure Modal open-craft/frontend-app-authoring#15 for details about section configuration modal.
See original PR: feat: add auto scroll to new sections when created open-craft/frontend-app-authoring#14 for details about auto scroll to element.
See original PR: feat: add drag n drop functionality to section cards open-craft/frontend-app-authoring#17 for details about section drag-n-drop.

Private-ref:

openedx-webhooks · 2023-12-07T07:33:49Z

Thanks for the pull request, @navinkarkera! 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.

openedx-webhooks · 2023-12-07T07:34:25Z

I've created issue TNL-11287 in the private 2U Jira.

codecov · 2023-12-07T07:41:04Z

Codecov Report

Attention: 43 lines in your changes are missing coverage. Please review.

Comparison is base (f938d08) 89.27% compared to head (c873e67) 89.56%.

Files	Patch %	Lines
src/course-outline/data/thunk.js	89.20%	15 Missing ⚠️
src/course-outline/hooks.jsx	73.46%	13 Missing ⚠️
src/course-outline/utils.jsx	87.17%	5 Missing ⚠️
src/course-outline/card-header/CardHeader.jsx	90.00%	3 Missing ⚠️
src/course-outline/CourseOutline.jsx	84.61%	2 Missing ⚠️
src/course-outline/configure-modal/BasicTab.jsx	87.50%	1 Missing ⚠️
.../course-outline/configure-modal/ConfigureModal.jsx	94.44%	1 Missing ⚠️
src/course-outline/publish-modal/PublishModal.jsx	92.85%	1 Missing ⚠️
src/course-outline/section-card/SectionCard.jsx	97.29%	1 Missing ⚠️
src/hooks.js	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #733      +/-   ##
==========================================
+ Coverage   89.27%   89.56%   +0.29%     
==========================================
  Files         476      495      +19     
  Lines        7568     8069     +501     
  Branches     1619     1697      +78     
==========================================
+ Hits         6756     7227     +471     
- Misses        784      815      +31     
+ Partials       28       27       -1

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

KristinAoki · 2023-12-08T15:35:06Z

src/course-outline/delete-modal/DeleteModal.jsx

+  return (
+    <AlertModal
+      title={intl.formatMessage(messages.title)}
+      variant="warning"


No variant is needed. All delete modals should use the derfault variant

KristinAoki · 2023-12-08T15:35:50Z

src/course-outline/delete-modal/messages.js

+  },
+  deleteButton: {
+    id: 'course-authoring.course-outline.delete-modal.button.delete',
+    defaultMessage: 'Yes, delete this section',


Suggested change

defaultMessage: 'Yes, delete this section',

defaultMessage: 'Delete',

KristinAoki · 2023-12-08T15:36:15Z

src/course-outline/empty-placeholder/EmptyPlaceholder.jsx

+        )}
+      >
+        <Button
+          variant="success"


Suggested change

variant="success"

variant="primary"

Can you also update them button to be small?

KristinAoki · 2023-12-08T15:49:09Z

src/course-outline/highlights-modal/HighlightsModal.jsx

+                })}
+              </p>
+              {Object.entries(initialFormValues).map(([key], index) => (
+                <FormikControl


Could these be changed to text boxes? This will help the user be able to fully read their highlight

@KristinAoki It renders an input text box as FormikControl uses Form.Control.

Sorry, I meant TextArea. It would be nice to see a multiline view.

Done. Looks like this now:

KristinAoki · 2023-12-08T15:49:59Z

src/course-outline/highlights-modal/messages.js

+  },
+  saveButton: {
+    id: 'course-authoring.course-outline.highlights-modal.button.save',
+    defaultMessage: 'Save highlights',


Suggested change

defaultMessage: 'Save highlights',

defaultMessage: 'Save',

KristinAoki · 2023-12-08T16:26:24Z

src/course-outline/section-card/SectionCard.scss

@@ -0,0 +1,34 @@
+.section-card {


Make div bakcground $light-100

KristinAoki · 2023-12-08T16:27:30Z

src/course-outline/section-card/SectionCard.jsx

+            onClick={handleOpenHighlightsModal}
+          >
+            <Badge className="highlights-badge">{highlights.length}</Badge>
+            <p className="m-0 text-black">Section highlights</p>


This text needs to use intl

KristinAoki · 2023-12-08T16:59:39Z

src/course-outline/CourseOutline.jsx

@@ -97,6 +133,37 @@ const CourseOutline = ({ courseId }) => {
                      statusBarData={statusBarData}
                      openEnableHighlightsModal={openEnableHighlightsModal}
                    />
+                    <div className="pt-4">
+                      {sectionsList.length ? (


Should this be sectionList.length > 0?

It is same, 0 is considered a falsy value.

KristinAoki · 2023-12-08T17:01:53Z

src/course-outline/publish-modal/PublishModal.jsx

+        {subSections.filter(subSection => subSection.hasChanges).map((subSection) => {
+          const units = subSection.childInfo.children.filter(unit => unit.hasChanges);
+
+          return units.length ? (


Same question as section lists?

It is same, 0 is considered a falsy value.

KristinAoki · 2023-12-08T17:02:44Z

src/course-outline/publish-modal/PublishModal.jsx

I will do another pass on the modal functionality when the subsections are added to the outline.

bradenmacdonald · 2023-12-08T22:50:08Z

src/course-outline/CourseOutline.jsx

  } = useCourseOutline({ courseId });

+  document.title = getPageHeadTitle(courseName, intl.formatMessage(messages.headingTitle));


Nit: IMHO it's better to use <Helmet> to set the title rather than writing to document.title directly.

Example:

https://github.com/openedx/frontend-app-course-authoring/blob/195c9e416c99bb8e53fc469d57887acf245a029c/src/taxonomy/taxonomy-detail/TaxonomyDetailPage.jsx#L82-L84

bradenmacdonald · 2023-12-08T23:03:50Z

src/course-outline/data/api.js

+/**
+ * Get course section
+ * @param {string} sectionId
+ * @returns {Promise<Object>}


Can you put the actual type here? Object is not very useful.

Nit/tip: Also, it's not your code but there are some major problems with the JSDoc comments in the other functions above. if you put // @ts-check on the first line of this function, you can easily see all the errors using an IDE like VS Code.

e.g.

Should be:

/** * Get course best practices. * @param {{courseId: string, excludeGraded: boolean, all: boolean}} options * @returns {Promise<{isSelfPaced: boolean, sections: any }>} */ export async function getCourseBestPractices({ courseId, excludeGraded, all, }) {

You don't have to change that now, but I do strongly recommend putting in the // @ts-check so that you're not adding more errors.

@bradenmacdonald Thanks! Actually, even this is not my code 😅. But yes, I should have checked these docs. Will try to fix them, atleast in course-outline related files.

haha I see. Well thanks for making those fixes anyways :)
I'm not planning to do any further review here, since @KristinAoki has it covered. But let me know if you do have specific questions I can help with.

bradenmacdonald · 2023-12-08T23:11:27Z

src/course-outline/constants.js

+export const SECTION_BADGE_STATUTES = {
+  live: 'live',
+  publishedNotLive: 'published_not_live',
+  staffOnly: 'staff_only',
+  draft: 'draft',
+};


Suggested change

export const SECTION_BADGE_STATUTES = {

live: 'live',

publishedNotLive: 'published_not_live',

staffOnly: 'staff_only',

draft: 'draft',

};

export const SECTION_BADGE_STATUTES = /** @type {const} */ ({

live: 'live',

publishedNotLive: 'published_not_live',

staffOnly: 'staff_only',

draft: 'draft',

});

Do this and you'll get the correct types :)

bradenmacdonald · 2023-12-08T23:13:10Z

src/course-outline/utils.jsx

+ * @param {bool} visibleToStaffOnly - value from section info
+ * @param {string} visibilityState - value from section info
+ * @param {bool} staffOnlyMessage - value from section info
+ * @returns {typeof SECTION_BADGE_STATUTES}


Suggested change

* @returns {typeof SECTION_BADGE_STATUTES}

* @returns {SECTION_BADGE_STATUTES[keyof SECTION_BADGE_STATUTES]}

What you have is wrong - it's returning an Object when this is a string. Use this ^ along with the other fix I suggested and you actually get the right types:

Throw in // @ts-check at the top of utils.jsx and SectionCard.tsx and you can see these issues more easily in VS Code.

navinkarkera · 2023-12-11T15:40:05Z

@KristinAoki @bradenmacdonald I have addressed your comments

@KristinAoki Also combined few PRs (linked in description) for you to review and merge them in a single go as discussed in the meeting. Please let me know before merging, I'll squash the commits according to the feature so that we can preserve all authors commits.

KristinAoki · 2023-12-11T16:27:37Z

src/course-outline/section-card/SectionCard.jsx

@@ -103,6 +103,7 @@ const SectionCard = forwardRef(({
        onEditSubmit={handleEditSubmit}
        isDisabledEditField={savingStatus === RequestStatus.IN_PROGRESS}
        onClickDuplicate={onDuplicateSubmit}
+        namePrefix={'section'}


remove brackets

KristinAoki · 2023-12-11T17:39:34Z

src/course-outline/utils.jsx

@@ -110,7 +110,11 @@ const getHighlightsFormValues = (currentHighlights) => {
 };

 const scrollToElement = (ref) => {


Had a weird bug appear when editing this. Repro steps

Add a new section

Should scroll to the new section

Scroll up to another section and edit its visibility
Expected

Once everything finishes saving, page should remain on edited section
Actual

scrolls to the recently added section

This also happens when editing in the basic tab

@KristinAoki This should be fixed now. We will only scroll to element when required.

KristinAoki · 2023-12-11T17:40:58Z

src/course-outline/utils.jsx

@@ -110,7 +110,11 @@ const getHighlightsFormValues = (currentHighlights) => {
 };

 const scrollToElement = (ref) => {
-  ref.current?.scrollIntoView({ behavior: 'smooth' });
+  ref.current?.scrollIntoView({


Do we want this to scroll even if the element is already in view? When I have a section in the center of my screen and I add a new subsection, the page scrolls so that the new subsection is at the bottom of my screen and the add button is no longer visible.

@KristinAoki Updated to only scroll if element is not visible.

navinkarkera · 2023-12-13T15:54:26Z

@KristinAoki Addressed your comments about scrolling. We have also merged section drag-n-drop PR to this one. Please take a look. ~~I'll fix failed tests tomorrow morning.~~ Fixed

KristinAoki

Regarding the drag-and-drop, there is already a component in frontend-lib-content-components that does all of the configuration for the dnd library. It is being used in custom-pages for the page reordering.

navinkarkera · 2023-12-15T11:10:08Z

Regarding the drag-and-drop, there is already a component in frontend-lib-content-components that does all of the configuration for the dnd library. It is being used in custom-pages for the page reordering.

@KristinAoki Thanks, did not know this. Updated to use these components similar to custom-pages. The only difference now is that we have a drag handle instead of allowing the user to drag the section from any point. Even if it is a bit different than the design, it is better as it will reduce complexity while making subsections and units movable.

KristinAoki

Drag and drop for sections looks great!

navinkarkera · 2023-12-18T04:47:52Z

@KristinAoki Thanks! I think this is ready to be merged if it looks good to you. Just let me know before merging, I'll squash commits as per the features and we can rebase and merge it.

KristinAoki

Once code coverage is passing, I will approve the PR

* feat: [2u-259] add components * feat: [2u-259] fix sidebar * feat: [2u-259] add tests, fix links * feat: [2u-259] fix messages * feat: [2u-159] fix reducer and sidebar * feat: [2u-259] fix reducer * feat: [2u-259] remove warning from selectors * feat: [2u-259] remove indents --------- Co-authored-by: Vladislav Keblysh <[email protected]> feat: Course Outline - Sections list (#59) * feat: [2u-336] add tests * feat: [2u-271] fix button * feat: [2u-336] add component, refactor header * fix: [2u-342] fix translates and indents * fix: [2u-342] fix constants and expand block * feat: [2u-336] remove new section from menu --------- Co-authored-by: Vladislav Keblysh <[email protected]> feat: Course outline - Content empty (#72) * feat: [2u-324] add component * feat: [2u-324] add translates * feat: [2u-324] update tests * feat: [2u-324] update branch * fix: [2u-324] fixed empty handler feat: Course outline - Section Publish (#61) * feat: [2u-354] add publish modal, api and update tests * feat: [2u-354] refactor modal * fix: [2u-354] removed comments * fix: [2u-354] fix indents * fix: [2u-354] removed translates duplicates * fix: [2u-354] rename handlers feat: Course outline - Update section card (#71) * feat: [2u-615] update section card * fix: [2u-615] fix handler names * fix: [2u-615] fix indents * fix: [2u-615] add empty handler * fix: [2u-615] fix data test id name * fix: [2u-615] fix styles fix: [2u-696] add saving processing for higlights and enable highlights (openedx#78) feat: Course outline - Section Edit (#70) * feat: [2u-336] add tests * feat: [2u-271] fix button * feat: [2u-336] add component, refactor header * feat: [2u-342] add modal * fix: [2u-342] fix translates and indents * feat: [2u-342] add modal * feat: [2u-342] add api * feat: [2u-342] add tests and translates * feat: [2u-342] fix indents * fix: [2u-342] fix indents, variant and utils * feat: [2u-342] fixed slice, thunks, hooks * feat: [2u-354] add publish modal, api and update tests * feat: [2u-615] update section card * feat: [2u-348] add api, handlers, tests * feat: [2u-348] add description for api * fix: [2u-348] fix useEscapeClick * fix: [2u-348] remove useEffect from CardHeader * fix: [2u-348] fixed handlers and tests * fix: [2u-348] fixed handlers and tests --------- Co-authored-by: Vladislav Keblysh <[email protected]> feat: Course outline - Section Delete (#74) * feat: [2u-336] add tests * feat: [2u-271] fix button * feat: [2u-336] add component, refactor header * feat: [2u-342] add modal * fix: [2u-342] fix translates and indents * feat: [2u-342] add modal * feat: [2u-342] add api * feat: [2u-342] add tests and translates * feat: [2u-342] fix indents * fix: [2u-342] fix indents, variant and utils * feat: [2u-342] fixed slice, thunks, hooks * feat: [2u-354] add publish modal, api and update tests * feat: [2u-615] update section card * feat: [2u-348] add api, handlers, tests * feat: [2u-510] add delete api, add delete modal * fix: [2u-510] fixed tests --------- Co-authored-by: Vladislav Keblysh <[email protected]> feat: Course outline - Section duplicate (openedx#88) * feat: [2u-336] add tests * feat: [2u-271] fix button * feat: [2u-336] add component, refactor header * feat: [2u-342] add modal * fix: [2u-342] fix translates and indents * feat: [2u-342] add modal * feat: [2u-342] add api * feat: [2u-342] add tests and translates * feat: [2u-342] fix indents * fix: [2u-342] fix indents, variant and utils * feat: [2u-342] fixed slice, thunks, hooks * feat: [2u-354] add publish modal, api and update tests * feat: [2u-615] update section card * feat: [2u-348] add api, handlers, tests * feat: [2u-510] add delete api, add delete modal * feat: [2u-360] add api * feat: [2u-360] add slice * feat: [2u-360] add tests * fix: [2u-360] fixed tests --------- Co-authored-by: Vladislav Keblysh <[email protected]> fix: Course outline - Highlights links (openedx#89) * fix: fixed doc urls * fix: fixed components feat: Course outline - Collapse all sections (#75) * feat: added collapse all section logic * fix: fixed tests fix: final revision commits fix: increase code coverage on the page

Also refactor api and hooks fix: publish button behaviour and card header tests fix: warning in highlights and publish modal test fix: courseoutline tests test: add test for new section functionality fix(lint): lint issues refactor: remove unnecessary css in CardHeader refactor: rename emptyPlaceholder test file refactor: replace ternary operator with 'and' condition refactor: add black color to expand/collapse button refactor: display only changed subsection and units in publish modal refactor: update messages and css refactor: wrap urls in function call refactor: fix jsdoc types refactor: use helmet for document title

fix: rename util function and remove unused eslint comment fix: fix tests by mocking scrollIntoView function test: add assertion for checking call to mock function

refactor: update publish modal to handle subsections and units refactor: rename current section state and handlers refactor: generalize edit title for section, subsection and unit feat: generalize delete modal feat: generalize publish modal refactor: use currentSection and currentSubsection to improve delete item feat: generalize duplication functionality feat: generalize add new item for sections and subsections test: fix subsection tests fix: lint issues and test arguments test: fix card header, delete and publish modal tests fix: invalid use of delete subsection query for unit refactor: use current section for highlights modal feat: add auto scroll to subsection and improve scroll behaviour fix: jsdoc types

feat: use react-dnd library for drag and drop implementation style: fix linting issues fix: finalize section order on drop instead of hover fix: prevent same index drag to start request and restore state on error fix: restore sectionlist order fix: prevent drag event while editing the text style: fix linting issues test: fix failing tests test: add missing hooks to SectionCard component in test test: add wrapping to SectionCard test component test: add tests for checking the API that sets the ordering fix: merge scroll-to-bottom with drag and drop implementations fix: fix linting issues

test: update tests fix: scroll to element only when required test: fix subsection component render refactor: use textarea for highlights

refactor: subsection and drag component style refactor: subsection styling refactor: generalize message ids for card header

refactor: remove delete unit hook and thunk till unit list is implemented test: additional tests for sections test: additional tests for subsections test: replace query calls by button clicks

navinkarkera · 2023-12-20T13:13:41Z

@KristinAoki This is ready to be merged now. Squashed commits as per feature and authors as much as possible, please use rebase and merge option while merging.

openedx-webhooks · 2023-12-20T15:20:51Z

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

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Dec 7, 2023

navinkarkera mentioned this pull request Dec 7, 2023

feat: course outline section list open-craft/frontend-app-authoring#12

Closed

navinkarkera added the jira:2u We want an issue in the 2U Jira instance label Dec 7, 2023

navinkarkera requested a review from KristinAoki December 7, 2023 07:34

navinkarkera mentioned this pull request Dec 7, 2023

Implement Section Configure Modal open-craft/frontend-app-authoring#15

Merged

KristinAoki requested changes Dec 8, 2023

View reviewed changes

bradenmacdonald reviewed Dec 8, 2023

View reviewed changes

navinkarkera force-pushed the navin/course-outline/section branch from 61ff8f4 to 6567ed9 Compare December 11, 2023 14:35

navinkarkera changed the title ~~feat: course outline - sections list~~ feat: course outline - sections and subsections list Dec 11, 2023

navinkarkera force-pushed the navin/course-outline/section branch from a4567ff to f181ca1 Compare December 11, 2023 15:37

navinkarkera requested review from KristinAoki and bradenmacdonald December 11, 2023 15:41

KristinAoki requested changes Dec 11, 2023

View reviewed changes

navinkarkera force-pushed the navin/course-outline/section branch from a68964a to 213cd0f Compare December 13, 2023 15:47

navinkarkera requested a review from KristinAoki December 14, 2023 10:32

KristinAoki reviewed Dec 14, 2023

View reviewed changes

navinkarkera requested a review from KristinAoki December 15, 2023 11:10

KristinAoki reviewed Dec 15, 2023

View reviewed changes

KristinAoki reviewed Dec 18, 2023

View reviewed changes

navinkarkera force-pushed the navin/course-outline/section branch 3 times, most recently from a7814d9 to 62a7fb2 Compare December 19, 2023 14:48

navinkarkera force-pushed the navin/course-outline/section branch from fac196d to 7da6de8 Compare December 20, 2023 11:39

navinkarkera and others added 8 commits December 20, 2023 17:27

feat: add Section Configure

4b343d3

feat: add auto scroll to new sections when created

641497a

fix: rename util function and remove unused eslint comment fix: fix tests by mocking scrollIntoView function test: add assertion for checking call to mock function

fix: handle scrolling with drag-n-drop

e6d6c39

test: update tests fix: scroll to element only when required test: fix subsection component render refactor: use textarea for highlights

refactor: reuse drag-n-drop component from lib-components

b4f6221

refactor: subsection and drag component style refactor: subsection styling refactor: generalize message ids for card header

test: improve coverage for course outline

c873e67

refactor: remove delete unit hook and thunk till unit list is implemented test: additional tests for sections test: additional tests for subsections test: replace query calls by button clicks

navinkarkera force-pushed the navin/course-outline/section branch from 7da6de8 to c873e67 Compare December 20, 2023 11:58

KristinAoki approved these changes Dec 20, 2023

View reviewed changes

KristinAoki merged commit df532b3 into openedx:master Dec 20, 2023
6 checks passed

navinkarkera deleted the navin/course-outline/section branch July 17, 2024 14:01

	defaultMessage: 'Yes, delete this section',
	defaultMessage: 'Delete',

		} = useCourseOutline({ courseId });

		document.title = getPageHeadTitle(courseName, intl.formatMessage(messages.headingTitle));

	* @returns {typeof SECTION_BADGE_STATUTES}
	* @returns {SECTION_BADGE_STATUTES[keyof SECTION_BADGE_STATUTES]}

		@@ -110,7 +110,11 @@ const getHighlightsFormValues = (currentHighlights) => {
		};

		const scrollToElement = (ref) => {

feat: course outline - sections and subsections list #733

feat: course outline - sections and subsections list #733

Conversation

navinkarkera commented Dec 7, 2023 • edited Loading

openedx-webhooks commented Dec 7, 2023

openedx-webhooks commented Dec 7, 2023

codecov bot commented Dec 7, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navinkarkera commented Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

navinkarkera Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

navinkarkera commented Dec 13, 2023 • edited Loading

KristinAoki left a comment • edited Loading

Choose a reason for hiding this comment

navinkarkera commented Dec 15, 2023 • edited Loading

KristinAoki left a comment

Choose a reason for hiding this comment

navinkarkera commented Dec 18, 2023

KristinAoki left a comment

Choose a reason for hiding this comment

navinkarkera commented Dec 20, 2023

openedx-webhooks commented Dec 20, 2023

navinkarkera commented Dec 7, 2023 •

edited

Loading

codecov bot commented Dec 7, 2023 •

edited

Loading

navinkarkera commented Dec 11, 2023 •

edited

Loading

navinkarkera Dec 13, 2023 •

edited

Loading

navinkarkera commented Dec 13, 2023 •

edited

Loading

KristinAoki left a comment •

edited

Loading

navinkarkera commented Dec 15, 2023 •

edited

Loading