fix: scroll to element only when required

openedx · Dec 13, 2023 · 213cd0f · 213cd0f
1 parent a7c1ae0
commit 213cd0f
Show file tree

Hide file tree

Showing 4 changed files with 47 additions and 37 deletions.
diff --git a/src/course-outline/CourseOutline.jsx b/src/course-outline/CourseOutline.jsx
@@ -1,5 +1,5 @@
 import {
-  React, useState, useCallback, useEffect, useRef,
+  React, useState, useCallback, useEffect,
 } from 'react';
 import update from 'immutability-helper';
 import { DndProvider } from 'react-dnd';
@@ -40,10 +40,8 @@ import ConfigureModal from './configure-modal/ConfigureModal';
 import DeleteModal from './delete-modal/DeleteModal';
 import { useCourseOutline } from './hooks';
 import messages from './messages';
-import { scrollToElement } from './utils';
 
 const CourseOutline = ({ courseId }) => {
-  const listRef = useRef(null);
   const intl = useIntl();
 
   const {
@@ -98,17 +96,14 @@ const CourseOutline = ({ courseId }) => {
     title: processingNotificationTitle,
   } = useSelector(getProcessingNotification);
 
-  const moveSection = useCallback((dragIndex, hoverIndex, dragElement) => {
+  const moveSection = useCallback((dragIndex, hoverIndex) => {
     setSections((prevSections) => {
       const newList = update(prevSections, {
         $splice: [
           [dragIndex, 1],
           [hoverIndex, 0, prevSections[dragIndex]],
         ],
       });
-      // set listRef to element that was dragged to make sure scrolling
-      // uses the correct element while calculating visibility.
-      listRef.current = dragElement;
       return newList;
     });
   }, []);
@@ -123,13 +118,6 @@ const CourseOutline = ({ courseId }) => {
     setSections(sectionsList);
   }, [sectionsList]);
 
-  useEffect(() => {
-    // scrollToElement called in another useEffect to make sure sections are rendered first.
-    if (listRef.current) {
-      scrollToElement(listRef.current);
-    }
-  }, [sections]);
-
   if (isLoading) {
     // eslint-disable-next-line react/jsx-no-useless-fragment
     return <></>;
@@ -210,7 +198,6 @@ const CourseOutline = ({ courseId }) => {
                                 onNewSubsectionSubmit={handleNewSubsectionSubmit}
                                 moveSection={moveSection}
                                 finalizeSectionOrder={finalizeSectionOrder}
-                                ref={listRef}
                               >
                                 {section.childInfo.children.map((subsection) => (
                                   <SubsectionCard
@@ -222,7 +209,6 @@ const CourseOutline = ({ courseId }) => {
                                     onOpenDeleteModal={openDeleteModal}
                                     onEditSubmit={handleEditSubmit}
                                     onDuplicateSubmit={handleDuplicateSubsectionSubmit}
-                                    ref={listRef}
                                   />
                                 ))}
                               </SectionCard>

diff --git a/src/course-outline/data/thunk.js b/src/course-outline/data/thunk.js
@@ -129,12 +129,13 @@ export function fetchCourseReindexQuery(courseId, reindexLink) {
   };
 }
 
-export function fetchCourseSectionQuery(sectionId) {
+export function fetchCourseSectionQuery(sectionId, shouldScroll = false) {
   return async (dispatch) => {
     dispatch(updateFetchSectionLoadingStatus({ status: RequestStatus.IN_PROGRESS }));
 
     try {
       const data = await getCourseItem(sectionId);
+      data.shouldScroll = shouldScroll;
       dispatch(updateSectionList(data));
       dispatch(updateFetchSectionLoadingStatus({ status: RequestStatus.SUCCESSFUL }));
     } catch (error) {
@@ -307,6 +308,8 @@ export function duplicateSectionQuery(sectionId, courseBlockId) {
       courseBlockId,
       async (locator) => {
         const duplicatedItem = await getCourseItem(locator);
+        // Page should scroll to newly duplicated item.
+        duplicatedItem.shouldScroll = true;
         dispatch(duplicateSection({ id: sectionId, duplicatedItem }));
       },
     ));
@@ -318,7 +321,7 @@ export function duplicateSubsectionQuery(subsectionId, sectionId) {
     dispatch(duplicateCourseItemQuery(
       subsectionId,
       sectionId,
-      async () => dispatch(fetchCourseSectionQuery(sectionId)),
+      async () => dispatch(fetchCourseSectionQuery(sectionId, true)),
     ));
   };
 }
@@ -344,6 +347,8 @@ function addNewCourseItemQuery(parentLocator, category, displayName, addItemFn)
       ).then(async (result) => {
         if (result) {
           const data = await getCourseItem(result.locator);
+          // Page should scroll to newly created item.
+          data.shouldScroll = true;
           dispatch(addItemFn(data));
           dispatch(updateSavingStatus({ status: RequestStatus.SUCCESSFUL }));
           dispatch(hideProcessingNotification());

diff --git a/src/course-outline/section-card/SectionCard.jsx b/src/course-outline/section-card/SectionCard.jsx
@@ -1,5 +1,5 @@
 import React, {
-  forwardRef, useEffect, useState, useRef,
+  useEffect, useState, useRef,
 } from 'react';
 import { useDrag, useDrop } from 'react-dnd';
 import PropTypes from 'prop-types';
@@ -11,11 +11,11 @@ import { Add as IconAdd } from '@edx/paragon/icons';
 import { setCurrentItem, setCurrentSection } from '../data/slice';
 import { RequestStatus } from '../../data/constants';
 import CardHeader from '../card-header/CardHeader';
-import { getItemStatus } from '../utils';
+import { getItemStatus, scrollToElement } from '../utils';
 import messages from './messages';
 import ItemTypes from './itemTypes';
 
-const SectionCard = forwardRef(({
+const SectionCard = ({
   section,
   index,
   children,
@@ -30,7 +30,8 @@ const SectionCard = forwardRef(({
   onNewSubsectionSubmit,
   moveSection,
   finalizeSectionOrder,
-}, lastItemRef) => {
+}) => {
+  const currentRef = useRef(null);
   const intl = useIntl();
   const dispatch = useDispatch();
   const [isExpanded, setIsExpanded] = useState(isSectionsExpanded);
@@ -40,6 +41,13 @@ const SectionCard = forwardRef(({
     setIsExpanded(isSectionsExpanded);
   }, [isSectionsExpanded]);
 
+  useEffect(() => {
+    // if this items has been newly added, scroll to it.
+    if (currentRef.current && section.shouldScroll) {
+      scrollToElement(currentRef.current);
+    }
+  }, []);
+
   const {
     id,
     displayName,
@@ -52,8 +60,6 @@ const SectionCard = forwardRef(({
     highlights,
   } = section;
 
-  const moveRef = useRef(null);
-
   const [{ handlerId }, drop] = useDrop({
     accept: ItemTypes.SECTION,
     collect(monitor) {
@@ -62,7 +68,7 @@ const SectionCard = forwardRef(({
       };
     },
     hover(item, monitor) {
-      if (!moveRef.current) {
+      if (!currentRef.current) {
         return;
       }
       const dragIndex = item.index;
@@ -72,7 +78,7 @@ const SectionCard = forwardRef(({
         return;
       }
       // Determine rectangle on screen
-      const hoverBoundingRect = moveRef.current?.getBoundingClientRect();
+      const hoverBoundingRect = currentRef.current?.getBoundingClientRect();
       // Get vertical middle
       const hoverMiddleY = (hoverBoundingRect.bottom - hoverBoundingRect.top) / 2;
       // Determine mouse position
@@ -91,7 +97,7 @@ const SectionCard = forwardRef(({
         return;
       }
       // Time to actually perform the action
-      moveSection(dragIndex, hoverIndex, moveRef.current);
+      moveSection(dragIndex, hoverIndex);
       // Note: we're mutating the monitor item here!
       // Generally it's better to avoid mutations,
       // but it's good here for the sake of performance
@@ -117,7 +123,7 @@ const SectionCard = forwardRef(({
     },
   });
   const opacity = isDragging ? 0 : 1;
-  drag(drop(moveRef));
+  drag(drop(currentRef));
 
   const sectionStatus = getItemStatus({
     published,
@@ -166,9 +172,9 @@ const SectionCard = forwardRef(({
       data-testid="section-card"
       data-handler-id={handlerId}
       style={{ opacity }}
-      ref={moveRef}
+      ref={currentRef}
     >
-      <div ref={lastItemRef}>
+      <div>
         <CardHeader
           sectionId={id}
           title={displayName}
@@ -222,7 +228,7 @@ const SectionCard = forwardRef(({
       </div>
     </div>
   );
-});
+};
 
 SectionCard.defaultProps = {
   children: null,
@@ -239,6 +245,7 @@ SectionCard.propTypes = {
     visibilityState: PropTypes.string.isRequired,
     staffOnlyMessage: PropTypes.bool.isRequired,
     highlights: PropTypes.arrayOf(PropTypes.string).isRequired,
+    shouldScroll: PropTypes.bool,
   }).isRequired,
   index: PropTypes.number.isRequired,
   children: PropTypes.node,

diff --git a/src/course-outline/subsection-card/SubsectionCard.jsx b/src/course-outline/subsection-card/SubsectionCard.jsx
@@ -1,4 +1,4 @@
-import { forwardRef, useEffect, useState } from 'react';
+import { useEffect, useState, useRef } from 'react';
 import PropTypes from 'prop-types';
 import { useDispatch } from 'react-redux';
 import { useIntl } from '@edx/frontend-platform/i18n';
@@ -8,10 +8,10 @@ import { Add as IconAdd } from '@edx/paragon/icons';
 import { setCurrentItem, setCurrentSection, setCurrentSubsection } from '../data/slice';
 import { RequestStatus } from '../../data/constants';
 import CardHeader from '../card-header/CardHeader';
-import { getItemStatus } from '../utils';
+import { getItemStatus, scrollToElement } from '../utils';
 import messages from './messages';
 
-const SubsectionCard = forwardRef(({
+const SubsectionCard = ({
   section,
   subsection,
   children,
@@ -20,7 +20,8 @@ const SubsectionCard = forwardRef(({
   savingStatus,
   onOpenDeleteModal,
   onDuplicateSubmit,
-}, lastItemRef) => {
+}) => {
+  const currentRef = useRef(null);
   const intl = useIntl();
   const dispatch = useDispatch();
   const [isExpanded, setIsExpanded] = useState(false);
@@ -64,14 +65,23 @@ const SubsectionCard = forwardRef(({
     closeForm();
   };
 
+  useEffect(() => {
+    // if this items has been newly added, scroll to it.
+    // we need to check section.shouldScroll as whole section is fetched when a
+    // subsection is duplicated under it.
+    if (currentRef.current && (section.shouldScroll || subsection.shouldScroll)) {
+      scrollToElement(currentRef.current);
+    }
+  }, []);
+
   useEffect(() => {
     if (savingStatus === RequestStatus.SUCCESSFUL) {
       closeForm();
     }
   }, [savingStatus]);
 
   return (
-    <div className="subsection-card" data-testid="subsection-card" ref={lastItemRef}>
+    <div className="subsection-card" data-testid="subsection-card" ref={currentRef}>
       <CardHeader
         title={displayName}
         status={subsectionStatus}
@@ -107,7 +117,7 @@ const SubsectionCard = forwardRef(({
       )}
     </div>
   );
-});
+};
 
 SubsectionCard.defaultProps = {
   children: null,
@@ -123,6 +133,7 @@ SubsectionCard.propTypes = {
     visibleToStaffOnly: PropTypes.bool,
     visibilityState: PropTypes.string.isRequired,
     staffOnlyMessage: PropTypes.bool.isRequired,
+    shouldScroll: PropTypes.bool,
   }).isRequired,
   subsection: PropTypes.shape({
     id: PropTypes.string.isRequired,
@@ -133,6 +144,7 @@ SubsectionCard.propTypes = {
     visibleToStaffOnly: PropTypes.bool,
     visibilityState: PropTypes.string.isRequired,
     staffOnlyMessage: PropTypes.bool.isRequired,
+    shouldScroll: PropTypes.bool,
   }).isRequired,
   children: PropTypes.node,
   onOpenPublishModal: PropTypes.func.isRequired,