feat: allow dragging blocks across parents in outline #859

navinkarkera · 2024-03-02T08:10:45Z

EDX_PLATFORM_REPOSITORY: https://github.com/openedx/edx-platform.git
EDX_PLATFORM_VERSION: master

TUTOR_GROVE_WAFFLE_FLAGS:
  - name: contentstore.new_studio_mfe.use_new_course_outline_page
    everyone: true

Description:
This PR modifies drag-n-drop of elements in outline to support possibility of dragging elements across different parent elements.

Test instructions:

Get devstack up and running using make {lms,cms}-up.
Start npm devserver in this repo using npm start.
Go to outline page of any course via cms and update port to 2001.
Drag and drop sections, subsections and units and confirm that they can be dropped in other parent elements similar to legacy.
PR also updates move up and down menu action to support moving across parents.

Private-ref: BB-8634

Closes: #851

openedx-webhooks · 2024-03-02T08:10:50Z

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.

codecov · 2024-03-02T08:31:11Z

Codecov Report

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

Project coverage is 91.87%. Comparing base (6ae9cda) to head (ecf01cd).
Report is 6 commits behind head on master.

Files	Patch %	Lines
src/course-outline/drag-helper/utils.js	88.69%	12 Missing and 1 partial ⚠️
src/course-outline/drag-helper/DraggableList.jsx	90.58%	8 Missing ⚠️
.../course-outline/subsection-card/SubsectionCard.jsx	83.33%	2 Missing ⚠️
src/course-outline/page-alerts/PageAlerts.jsx	95.83%	1 Missing ⚠️
src/course-outline/section-card/SectionCard.jsx	88.88%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #859      +/-   ##
==========================================
- Coverage   91.88%   91.87%   -0.02%     
==========================================
  Files         562      574      +12     
  Lines        9776    10078     +302     
  Branches     2085     2157      +72     
==========================================
+ Hits         8983     9259     +276     
- Misses        766      791      +25     
- Partials       27       28       +1

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

mphilbrick211 · 2024-03-04T15:57:04Z

@openedx/2u-tnl

arbrandes · 2024-03-04T17:01:14Z

Is there a feature flag to set to get the new experience without going to port 2001 manually?

open-craft-grove · 2024-03-04T18:07:52Z

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

navinkarkera · 2024-03-07T11:49:57Z

@arbrandes No, we don't have a flag yet. You can set new_studio_mfe.use_new_home_page waffle
flag to go to the new MFE home page, but the individual courses still take you to legacy UI.

@KristinAoki Maybe we can add a flag similar to ENABLE_UNIT_PAGE in this repo and redirect user to new outline if it is set.

arbrandes · 2024-03-08T14:12:56Z

@navinkarkera, ok, and yes, I think an MFE-specific environment variable would be nice to have.

In the meantime, what would be the correct link from the sandbox course page? (Or from a tutor deployment, for that matter.)

KristinAoki · 2024-03-08T14:37:05Z

To get to the outline page you can enable contentstore.new_studio_mfe.use_new_course_outline_page. This will allow page redirects when you try to hit the legacy page.

arbrandes

Wow, this one's a doozy! Congrats! And it works quite nicely in the sandbox, too.

I do have a couple of nits, however.

arbrandes · 2024-03-08T16:36:19Z

src/course-outline/drag-helper/DraggableList.jsx

+    return null;
+  };
+
+  /* istanbul ignore next */


Was this very hard to test? I don't mean to block the PR because of it, but I feel like we should at least explain why we're not testing it in a comment.

The same goes for other instances in this PR.

@arbrandes Ahh, I meant to write a comment about it, but then forgot. I have ignored two functions from coverage, i.e., unitDragOver and subsectionDragOver. For some weird reason, dragEnd handler is not being triggered by dnd-kit while testing them.

I had already spent quite some time testing the functions related to moving/dragging blocks which doesn't work with jsdom easily as it does not implement getClientBoundingRect and renders all elements on top of each other at (0, 0) position. So I had to mock collisionDetection function which helped me test movement of blocks inside its own parent as well as across parents.

The main functions responsible to move units & subsections across parents are tested, just these functions which determine the new index and parent are ignored.

Thanks, it makes sense.

I still can't find the explanation in the code, though. Did you add it? If not, I would suggest adding it at the first occurrence of the istanbul ignore next of each file where this occurs. (I expect it's just this file.)

@arbrandes Done.

src/course-outline/drag-helper/DraggableList.jsx

src/course-outline/page-alerts/messages.js

open-craft-grove · 2024-03-11T08:06:47Z

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

navinkarkera · 2024-03-11T10:15:17Z

@arbrandes @KristinAoki Thanks! Addressed your feedback, can you please take a look once again.

arbrandes

Just looking for a code comment on why we're not testing some bits.

arbrandes · 2024-03-13T13:38:32Z

src/course-outline/drag-helper/DraggableList.jsx

+    return null;
+  };
+
+  /* istanbul ignore next */


Thanks, it makes sense.

I still can't find the explanation in the code, though. Did you add it? If not, I would suggest adding it at the first occurrence of the istanbul ignore next of each file where this occurs. (I expect it's just this file.)

arbrandes · 2024-03-13T13:42:31Z

src/course-outline/page-alerts/messages.js

@@ -57,6 +57,34 @@ const messages = defineMessages({
    id: 'course-authoring.course-outline.page-alert.generic-error.description',
    defaultMessage: 'Unable to {actionName} {type}. Please try again.',
  },
+  newFileAlertTitle: {
+    id: 'course-authoring.course-outline.page-alert.paste-alert.new-files.title',
+    defaultMessage: 'New {newFilesLen, plural, one {file} other {files}} added to Files.',


Pretty soon we'll start linting for messages that don't have descriptions. It's an important part of i18n, as it makes the lives of translators much easier. I'm not going to block PRs on this until openedx/frontend-build#516 merges, but consider this friendly heads-up. :)

@arbrandes That is great. Although I cannot add descriptions to all messages files in the repo, I added them to this file as a start.

open-craft-grove · 2024-03-14T07:06:55Z

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

navinkarkera · 2024-03-19T10:00:44Z

@KristinAoki I think this is ready to be merged (squash and merge).
cc: @arbrandes

arbrandes

Looks great, thanks! (In particular, thank you for adding descriptions to the i18n messages!)

openedx-webhooks · 2024-03-19T14:25:06Z

@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 Mar 2, 2024

navinkarkera force-pushed the navin/course-outline/dragndrop branch from 704792b to da9fd27 Compare March 2, 2024 08:22

navinkarkera marked this pull request as ready for review March 2, 2024 08:26

navinkarkera requested a review from a team as a code owner March 2, 2024 08:26

navinkarkera force-pushed the navin/course-outline/dragndrop branch from da9fd27 to 4c724b6 Compare March 4, 2024 11:05

arbrandes self-requested a review March 4, 2024 16:59

arbrandes added the create-sandbox open-craft-grove should create a sandbox environment from this PR label Mar 4, 2024

KristinAoki approved these changes Mar 8, 2024

View reviewed changes

arbrandes requested changes Mar 8, 2024

View reviewed changes

KristinAoki reviewed Mar 8, 2024

View reviewed changes

src/course-outline/page-alerts/messages.js Outdated Show resolved Hide resolved

navinkarkera force-pushed the navin/course-outline/dragndrop branch from 4c724b6 to d201cb4 Compare March 11, 2024 07:18

navinkarkera mentioned this pull request Mar 12, 2024

Some problems with drag and drop functionality #851

Closed

KristinAoki approved these changes Mar 12, 2024

View reviewed changes

arbrandes requested changes Mar 13, 2024

View reviewed changes

navinkarkera added 6 commits March 14, 2024 11:33

fix: block highlight and status for unscheduled course

2933048

feat: show alerts related to files after pasting unit

7c4021f

refactor: rename paste notices and add view files option to alert

1424db5

refactor: remove additional visibility state check

90061c1

refactor: allow dragging blocks across parents in outline

3f8ffb6

fixup! refactor: allow dragging blocks across parents in outline

6b400d0

navinkarkera added 19 commits March 14, 2024 11:33

fixup! refactor: allow dragging blocks across parents in outline

731d2c0

fixup! refactor: allow dragging blocks across parents in outline

09593cc

fixup! refactor: allow dragging blocks across parents in outline

bd9e971

refactor: update order list api

3166045

refactor: fetch section to accept a list

46feea1

fixup! refactor: update order list api

5c456f3

refactor: drag functions

bd9d049

refactor: fix move up & down button

356de42

refactor: fix move up & down for units

1511a94

refactor: check all possible conditions before moving

ca5cefe

fixup! refactor: check all possible conditions before moving

c2d9657

test: drag and move option tests

1baf79e

fixup! test: drag and move option tests

05f1712

fix: lint issues

b404d3b

fix: proptypes

bd5e363

test: ignore dragover functions in coverage report

125ad7a

refactor: improve items proptype in draggableList component

40a9dbe

refactor: add comment for ignored functions from coverage

21ef35e

chore: add description text for page alerts i18n messages

a75dfbd

navinkarkera force-pushed the navin/course-outline/dragndrop branch from d201cb4 to a75dfbd Compare March 14, 2024 06:28

navinkarkera requested a review from a team as a code owner March 14, 2024 06:28

navinkarkera requested a review from arbrandes March 14, 2024 06:30

refactor: update draft status icon

ecf01cd

arbrandes approved these changes Mar 19, 2024

View reviewed changes

arbrandes merged commit da68fb8 into openedx:master Mar 19, 2024
6 checks passed

navinkarkera deleted the navin/course-outline/dragndrop branch July 17, 2024 14:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow dragging blocks across parents in outline #859

feat: allow dragging blocks across parents in outline #859

navinkarkera commented Mar 2, 2024 •

edited

Loading

openedx-webhooks commented Mar 2, 2024 •

edited

Loading

codecov bot commented Mar 2, 2024 •

edited

Loading

mphilbrick211 commented Mar 4, 2024

arbrandes commented Mar 4, 2024

open-craft-grove commented Mar 4, 2024

navinkarkera commented Mar 7, 2024

arbrandes commented Mar 8, 2024

KristinAoki commented Mar 8, 2024

arbrandes left a comment

arbrandes Mar 8, 2024

navinkarkera Mar 11, 2024 •

edited

Loading

arbrandes Mar 13, 2024

navinkarkera Mar 14, 2024

open-craft-grove commented Mar 11, 2024

navinkarkera commented Mar 11, 2024

arbrandes left a comment

arbrandes Mar 13, 2024

arbrandes Mar 13, 2024

navinkarkera Mar 14, 2024

open-craft-grove commented Mar 14, 2024

navinkarkera commented Mar 19, 2024

arbrandes left a comment

openedx-webhooks commented Mar 19, 2024

feat: allow dragging blocks across parents in outline #859

feat: allow dragging blocks across parents in outline #859

Conversation

navinkarkera commented Mar 2, 2024 • edited Loading

openedx-webhooks commented Mar 2, 2024 • edited Loading

codecov bot commented Mar 2, 2024 • edited Loading

Codecov Report

mphilbrick211 commented Mar 4, 2024

arbrandes commented Mar 4, 2024

open-craft-grove commented Mar 4, 2024

navinkarkera commented Mar 7, 2024

arbrandes commented Mar 8, 2024

KristinAoki commented Mar 8, 2024

arbrandes left a comment

Choose a reason for hiding this comment

arbrandes Mar 8, 2024

Choose a reason for hiding this comment

navinkarkera Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

arbrandes Mar 13, 2024

Choose a reason for hiding this comment

navinkarkera Mar 14, 2024

Choose a reason for hiding this comment

open-craft-grove commented Mar 11, 2024

navinkarkera commented Mar 11, 2024

arbrandes left a comment

Choose a reason for hiding this comment

arbrandes Mar 13, 2024

Choose a reason for hiding this comment

arbrandes Mar 13, 2024

Choose a reason for hiding this comment

navinkarkera Mar 14, 2024

Choose a reason for hiding this comment

open-craft-grove commented Mar 14, 2024

navinkarkera commented Mar 19, 2024

arbrandes left a comment

Choose a reason for hiding this comment

openedx-webhooks commented Mar 19, 2024

navinkarkera commented Mar 2, 2024 •

edited

Loading

openedx-webhooks commented Mar 2, 2024 •

edited

Loading

codecov bot commented Mar 2, 2024 •

edited

Loading

navinkarkera Mar 11, 2024 •

edited

Loading