feat: search modal refinements (#959)

* feat: More spacing between search bar and selectmenu
* feat: Autofocus search field when modal opens
* feat: Fix issues with scroll to search result
This includes the following:
  - The target search element is aligned to the top of the page when scrolling to it
  - Makes sure the section/subsection is expanded in order to scroll to result
* fix: Match focus border radius with button's
* fix: Only expand (sub)section with search result

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

@@ -42,10 +42,36 @@ const SectionCard = ({
   const intl = useIntl();
   const dispatch = useDispatch();
   const { activeId, overId } = useContext(DragContext);
-  const [isExpanded, setIsExpanded] = useState(isSectionsExpanded);
   const [searchParams] = useSearchParams();
   const locatorId = searchParams.get('show');
   const isScrolledToElement = locatorId === section.id;
+
+  // Expand the section if a search result should be shown/scrolled to
+  const containsSearchResult = () => {
+    if (locatorId) {
+      const subsections = section.childInfo?.children;
+      if (subsections) {
+        for (let i = 0; i < subsections.length; i++) {
+          const subsection = subsections[i];
+
+          // Check if the search result is one of the subsections
+          const matchedSubsection = subsection.id === locatorId;
+          if (matchedSubsection) {
+            return true;
+          }
+
+          // Check if the search result is one of the units
+          const matchedUnit = !!subsection.childInfo?.children?.filter((child) => child.id === locatorId).length;
+          if (matchedUnit) {
+            return true;
+          }
+        }
+      }
+    }
+
+    return false;
+  };
+  const [isExpanded, setIsExpanded] = useState(containsSearchResult() || isSectionsExpanded);
   const [isFormOpen, openForm, closeForm] = useToggle(false);
   const namePrefix = 'section';
 
@@ -75,10 +101,18 @@ const SectionCard = ({
 
   useEffect(() => {
     if (currentRef.current && (section.shouldScroll || isScrolledToElement)) {
-      scrollToElement(currentRef.current);
+      // Align element closer to the top of the screen if scrolling for search result
+      const alignWithTop = !!isScrolledToElement;
+      scrollToElement(currentRef.current, alignWithTop);
     }
   }, [isScrolledToElement]);
 
+  useEffect(() => {
+    // If the locatorId is set/changed, we need to make sure that the section is expanded
+    // if it contains the result, in order to scroll to it
+    setIsExpanded((prevState) => containsSearchResult() || prevState);
+  }, [locatorId, setIsExpanded]);
+
   // re-create actions object for customizations
   const actions = { ...sectionActions };
   // add actions to control display of move up & down menu buton.
@@ -253,6 +287,20 @@ SectionCard.propTypes = {
       duplicable: PropTypes.bool.isRequired,
     }).isRequired,
     isHeaderVisible: PropTypes.bool,
+    childInfo: PropTypes.shape({
+      children: PropTypes.arrayOf(
+        PropTypes.shape({
+          id: PropTypes.string.isRequired,
+          childInfo: PropTypes.shape({
+            children: PropTypes.arrayOf(
+              PropTypes.shape({
+                id: PropTypes.string.isRequired,
+              }),
+            ).isRequired,
+          }).isRequired,
+        }),
+      ).isRequired,
+    }).isRequired,
   }).isRequired,
   isSelfPaced: PropTypes.bool.isRequired,
   isCustomRelativeDatesActive: PropTypes.bool.isRequired,

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

@@ -1,4 +1,5 @@
 import React from 'react';
+import { MemoryRouter } from 'react-router-dom';
 import {
   act, render, fireEvent, within,
 } from '@testing-library/react';
@@ -15,6 +16,40 @@ import SectionCard from './SectionCard';
 // eslint-disable-next-line no-unused-vars
 let axiosMock;
 let store;
+const mockPathname = '/foo-bar';
+
+jest.mock('react-router-dom', () => ({
+  ...jest.requireActual('react-router-dom'),
+  useLocation: () => ({
+    pathname: mockPathname,
+  }),
+}));
+
+const unit = {
+  id: 'unit-1',
+};
+
+const subsection = {
+  id: '123',
+  displayName: 'Subsection Name',
+  category: 'sequential',
+  published: true,
+  visibilityState: 'live',
+  hasChanges: false,
+  actions: {
+    draggable: true,
+    childAddable: true,
+    deletable: true,
+    duplicable: true,
+  },
+  isHeaderVisible: true,
+  releasedToStudents: true,
+  childInfo: {
+    children: [{
+      id: unit.id,
+    }],
+  },
+};
 
 const section = {
   id: '123',
@@ -31,37 +66,49 @@ const section = {
     duplicable: true,
   },
   isHeaderVisible: true,
+  childInfo: {
+    children: [{
+      id: subsection.id,
+      childInfo: {
+        children: [{
+          id: unit.id,
+        }],
+      },
+    }],
+  },
 };
 
 const onEditSectionSubmit = jest.fn();
 
 const queryClient = new QueryClient();
 
-const renderComponent = (props) => render(
-  <AppProvider store={store}>
+const renderComponent = (props, entry = '/') => render(
+  <AppProvider store={store} wrapWithRouter={false}>
     <QueryClientProvider client={queryClient}>
-      <IntlProvider locale="en">
-        <SectionCard
-          section={section}
-          index={1}
-          canMoveItem={jest.fn()}
-          onOrderChange={jest.fn()}
-          onOpenPublishModal={jest.fn()}
-          onOpenHighlightsModal={jest.fn()}
-          onOpenDeleteModal={jest.fn()}
-          onOpenConfigureModal={jest.fn()}
-          savingStatus=""
-          onEditSectionSubmit={onEditSectionSubmit}
-          onDuplicateSubmit={jest.fn()}
-          isSectionsExpanded
-          onNewSubsectionSubmit={jest.fn()}
-          isSelfPaced={false}
-          isCustomRelativeDatesActive={false}
-          {...props}
-        >
-          <span>children</span>
-        </SectionCard>
-      </IntlProvider>
+      <MemoryRouter initialEntries={[entry]}>
+        <IntlProvider locale="en">
+          <SectionCard
+            section={section}
+            index={1}
+            canMoveItem={jest.fn()}
+            onOrderChange={jest.fn()}
+            onOpenPublishModal={jest.fn()}
+            onOpenHighlightsModal={jest.fn()}
+            onOpenDeleteModal={jest.fn()}
+            onOpenConfigureModal={jest.fn()}
+            savingStatus=""
+            onEditSectionSubmit={onEditSectionSubmit}
+            onDuplicateSubmit={jest.fn()}
+            isSectionsExpanded
+            onNewSubsectionSubmit={jest.fn()}
+            isSelfPaced={false}
+            isCustomRelativeDatesActive={false}
+            {...props}
+          >
+            <span>children</span>
+          </SectionCard>
+        </IntlProvider>
+      </MemoryRouter>
     </QueryClientProvider>
   </AppProvider>,
 );
@@ -148,4 +195,38 @@ describe('<SectionCard />', () => {
     expect(within(element).queryByTestId('section-card-header__menu-delete-button')).not.toBeInTheDocument();
     expect(queryByTestId('new-subsection-button')).not.toBeInTheDocument();
   });
+
+  it('check extended section when URL "show" param in subsection under section', async () => {
+    const collapsedSections = { ...section };
+    collapsedSections.isSectionsExpanded = false;
+    const { findByTestId } = renderComponent(collapsedSections, `?show=${subsection.id}`);
+
+    const cardSubsections = await findByTestId('section-card__subsections');
+    const newSubsectionButton = await findByTestId('new-subsection-button');
+    expect(cardSubsections).toBeInTheDocument();
+    expect(newSubsectionButton).toBeInTheDocument();
+  });
+
+  it('check extended section when URL "show" param in unit under section', async () => {
+    const collapsedSections = { ...section };
+    collapsedSections.isSectionsExpanded = false;
+    const { findByTestId } = renderComponent(collapsedSections, `?show=${unit.id}`);
+
+    const cardSubsections = await findByTestId('section-card__subsections');
+    const newSubsectionButton = await findByTestId('new-subsection-button');
+    expect(cardSubsections).toBeInTheDocument();
+    expect(newSubsectionButton).toBeInTheDocument();
+  });
+
+  it('check not extended section when URL "show" param not in section', async () => {
+    const randomId = 'random-id';
+    const collapsedSections = { ...section };
+    collapsedSections.isSectionsExpanded = false;
+    const { queryByTestId } = renderComponent(collapsedSections, `?show=${randomId}`);
+
+    const cardSubsections = await queryByTestId('section-card__subsections');
+    const newSubsectionButton = await queryByTestId('new-subsection-button');
+    expect(cardSubsections).toBeNull();
+    expect(newSubsectionButton).toBeNull();
+  });
 });

src/course-outline/subsection-card/SubsectionCard.jsx

@@ -72,7 +72,15 @@ const SubsectionCard = ({
   actions.allowMoveUp = !isEmpty(moveUpDetails);
   actions.allowMoveDown = !isEmpty(moveDownDetails);
 
-  const [isExpanded, setIsExpanded] = useState(locatorId ? isScrolledToElement : !isHeaderVisible);
+  // Expand the subsection if a search result should be shown/scrolled to
+  const containsSearchResult = () => {
+    if (locatorId) {
+      return !!subsection.childInfo?.children?.filter((child) => child.id === locatorId).length;
+    }
+
+    return false;
+  };
+  const [isExpanded, setIsExpanded] = useState(containsSearchResult() || !isHeaderVisible);
   const subsectionStatus = getItemStatus({
     published,
     visibilityState,
@@ -132,10 +140,18 @@ const SubsectionCard = ({
     // 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 || isScrolledToElement)) {
-      scrollToElement(currentRef.current);
+      // Align element closer to the top of the screen if scrolling for search result
+      const alignWithTop = !!isScrolledToElement;
+      scrollToElement(currentRef.current, alignWithTop);
     }
   }, [isScrolledToElement]);
 
+  useEffect(() => {
+    // If the locatorId is set/changed, we need to make sure that the subsection is expanded
+    // if it contains the result, in order to scroll to it
+    setIsExpanded((prevState) => (containsSearchResult() || prevState));
+  }, [locatorId, setIsExpanded]);
+
   useEffect(() => {
     if (savingStatus === RequestStatus.SUCCESSFUL) {
       closeForm();
@@ -264,6 +280,13 @@ SubsectionCard.propTypes = {
       duplicable: PropTypes.bool.isRequired,
     }).isRequired,
     isHeaderVisible: PropTypes.bool,
+    childInfo: PropTypes.shape({
+      children: PropTypes.arrayOf(
+        PropTypes.shape({
+          id: PropTypes.string.isRequired,
+        }),
+      ).isRequired,
+    }).isRequired,
   }).isRequired,
   children: PropTypes.node,
   isSelfPaced: PropTypes.bool.isRequired,

src/course-outline/subsection-card/SubsectionCard.test.jsx

@@ -32,13 +32,8 @@ const clipboardBroadcastChannelMock = {
 
 global.BroadcastChannel = jest.fn(() => clipboardBroadcastChannelMock);
 
-const section = {
-  id: '123',
-  displayName: 'Section Name',
-  published: true,
-  visibilityState: 'live',
-  hasChanges: false,
-  highlights: ['highlight 1', 'highlight 2'],
+const unit = {
+  id: 'unit-1',
 };
 
 const subsection = {
@@ -56,6 +51,25 @@ const subsection = {
   },
   isHeaderVisible: true,
   releasedToStudents: true,
+  childInfo: {
+    children: [{
+      id: unit.id,
+    }],
+  },
+};
+
+const section = {
+  id: '123',
+  displayName: 'Section Name',
+  published: true,
+  visibilityState: 'live',
+  hasChanges: false,
+  highlights: ['highlight 1', 'highlight 2'],
+  childInfo: {
+    children: [{
+      id: subsection.id,
+    }],
+  },
 };
 
 const onEditSubectionSubmit = jest.fn();
@@ -227,12 +241,22 @@ describe('<SubsectionCard />', () => {
     expect(await findByText(cardHeaderMessages.statusBadgeDraft.defaultMessage)).toBeInTheDocument();
   });
 
-  it('check extended section when URL has a "show" param', async () => {
-    const { findByTestId } = renderComponent(null, `?show=${section.id}`);
+  it('check extended subsection when URL "show" param in subsection', async () => {
+    const { findByTestId } = renderComponent(null, `?show=${unit.id}`);
 
     const cardUnits = await findByTestId('subsection-card__units');
     const newUnitButton = await findByTestId('new-unit-button');
     expect(cardUnits).toBeInTheDocument();
     expect(newUnitButton).toBeInTheDocument();
   });
+
+  it('check not extended subsection when URL "show" param not in subsection', async () => {
+    const randomId = 'random-id';
+    const { queryByTestId } = renderComponent(null, `?show=${randomId}`);
+
+    const cardUnits = await queryByTestId('subsection-card__units');
+    const newUnitButton = await queryByTestId('new-unit-button');
+    expect(cardUnits).toBeNull();
+    expect(newUnitButton).toBeNull();
+  });
 });

src/course-outline/unit-card/UnitCard.jsx

@@ -114,7 +114,9 @@ const UnitCard = ({
     // we need to check section.shouldScroll as whole section is fetched when a
     // unit is duplicated under it.
     if (currentRef.current && (section.shouldScroll || unit.shouldScroll || isScrolledToElement)) {
-      scrollToElement(currentRef.current);
+      // Align element closer to the top of the screen if scrolling for search result
+      const alignWithTop = !!isScrolledToElement;
+      scrollToElement(currentRef.current, alignWithTop);
     }
   }, [isScrolledToElement]);
 

src/course-outline/utils.jsx

@@ -165,12 +165,20 @@ const getHighlightsFormValues = (currentHighlights) => {
  * Method to scroll into view port, if it's outside the viewport
  *
  * @param {Object} target - DOM Element
+ * @param {boolean} alignWithTop (optional) - Whether top of the target will be aligned to
+ *                                            the top of viewpoint. (default: false)
  * @returns {undefined}
  */
-const scrollToElement = target => {
+const scrollToElement = (target, alignWithTop = false) => {
   if (target.getBoundingClientRect().bottom > window.innerHeight) {
-    //  The bottom of the target will be aligned to the bottom of the visible area of the scrollable ancestor.
-    target.scrollIntoView({ behavior: 'smooth', block: 'end', inline: 'nearest' });
+    // if alignWithTop is set, the top of the target will be aligned to the top of visible area
+    // of the scrollable ancestor, Otherwise, the bottom of the target will be aligned to the
+    // bottom of the visible area of the scrollable ancestor.
+    target.scrollIntoView({
+      behavior: 'smooth',
+      block: alignWithTop ? 'start' : 'end',
+      inline: 'nearest',
+    });
   }
 
   // Target is outside the view from the top

src/search-modal/SearchKeywordsField.jsx

@@ -15,14 +15,21 @@ const SearchKeywordsField = (props) => {
   const { searchKeywords, setSearchKeywords } = useSearchContext();
 
   return (
-    <SearchField
+    <SearchField.Advanced
       onSubmit={setSearchKeywords}
       onChange={setSearchKeywords}
       onClear={() => setSearchKeywords('')}
       value={searchKeywords}
       className={props.className}
-      placeholder={intl.formatMessage(messages.inputPlaceholder)}
-    />
+    >
+      <SearchField.Label />
+      <SearchField.Input
+        autoFocus
+        placeholder={intl.formatMessage(messages.inputPlaceholder)}
+      />
+      <SearchField.ClearButton />
+      <SearchField.SubmitButton />
+    </SearchField.Advanced>
   );
 };
 

src/search-modal/SearchModal.scss

@@ -31,6 +31,11 @@
       // The current Open edX theme makes the search field square but the button round and it looks bad. We need this
       // hacky override until the theme is fixed to be more consistent.
       border-radius: 0;
+
+      // Needed so the the focus borders matches the button's borders
+      &:focus::before {
+        border-radius: 0;
+      }
     }
   }
 

src/search-modal/SearchUI.jsx

@@ -37,7 +37,7 @@ const SearchUI = (props) => {
       <ModalDialog.Header style={{ zIndex: 9 }} className="border-bottom">
         <ModalDialog.Title><FormattedMessage {...messages.title} /></ModalDialog.Title>
         <div className="d-flex mt-3">
-          <SearchKeywordsField className="flex-grow-1 mr-1" />
+          <SearchKeywordsField className="flex-grow-1 mr-2" />
           <SelectMenu variant="primary">
             <MenuItem
               onClick={switchToThisCourse}