From 6296668ffd0638fed13ba79dcf7e6f113c808908 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Tue, 23 Apr 2024 10:29:44 +0300 Subject: [PATCH 1/5] feat: More spacing between search bar and selectmenu --- src/search-modal/SearchUI.jsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/search-modal/SearchUI.jsx b/src/search-modal/SearchUI.jsx index be85e97632..c95c69bb80 100644 --- a/src/search-modal/SearchUI.jsx +++ b/src/search-modal/SearchUI.jsx @@ -37,7 +37,7 @@ const SearchUI = (props) => {
- + Date: Tue, 23 Apr 2024 11:51:09 +0300 Subject: [PATCH 2/5] feat: Autofocus search field when modal opens --- src/search-modal/SearchKeywordsField.jsx | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/search-modal/SearchKeywordsField.jsx b/src/search-modal/SearchKeywordsField.jsx index 809dd7b430..b630b3998e 100644 --- a/src/search-modal/SearchKeywordsField.jsx +++ b/src/search-modal/SearchKeywordsField.jsx @@ -15,14 +15,21 @@ const SearchKeywordsField = (props) => { const { searchKeywords, setSearchKeywords } = useSearchContext(); return ( - setSearchKeywords('')} value={searchKeywords} className={props.className} - placeholder={intl.formatMessage(messages.inputPlaceholder)} - /> + > + + + + + ); }; From 5715dcaa836a0092e4d9d33a174064ce93340405 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Wed, 24 Apr 2024 16:42:08 +0300 Subject: [PATCH 3/5] 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 --- src/course-outline/section-card/SectionCard.jsx | 12 ++++++++++-- .../subsection-card/SubsectionCard.jsx | 13 +++++++++++-- src/course-outline/unit-card/UnitCard.jsx | 4 +++- src/course-outline/utils.jsx | 14 +++++++++++--- 4 files changed, 35 insertions(+), 8 deletions(-) diff --git a/src/course-outline/section-card/SectionCard.jsx b/src/course-outline/section-card/SectionCard.jsx index 55089158fd..4eea3175d9 100644 --- a/src/course-outline/section-card/SectionCard.jsx +++ b/src/course-outline/section-card/SectionCard.jsx @@ -42,10 +42,10 @@ 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; + const [isExpanded, setIsExpanded] = useState(locatorId ? !!locatorId : isSectionsExpanded); const [isFormOpen, openForm, closeForm] = useToggle(false); const namePrefix = 'section'; @@ -75,10 +75,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 + // in order to scroll to search result, otherwise leave it as is. + setIsExpanded((prevState) => (locatorId ? !!locatorId : prevState)); + }, [locatorId, setIsExpanded]); + // re-create actions object for customizations const actions = { ...sectionActions }; // add actions to control display of move up & down menu buton. diff --git a/src/course-outline/subsection-card/SubsectionCard.jsx b/src/course-outline/subsection-card/SubsectionCard.jsx index 12bee0aec3..b798fa5b99 100644 --- a/src/course-outline/subsection-card/SubsectionCard.jsx +++ b/src/course-outline/subsection-card/SubsectionCard.jsx @@ -72,7 +72,8 @@ 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 [isExpanded, setIsExpanded] = useState(locatorId ? !!locatorId : !isHeaderVisible); const subsectionStatus = getItemStatus({ published, visibilityState, @@ -132,10 +133,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 + // in order to scroll to search result + setIsExpanded((prevState) => (locatorId ? !!locatorId : prevState)); + }, [locatorId, setIsExpanded]); + useEffect(() => { if (savingStatus === RequestStatus.SUCCESSFUL) { closeForm(); diff --git a/src/course-outline/unit-card/UnitCard.jsx b/src/course-outline/unit-card/UnitCard.jsx index 469f28e791..b828fe05af 100644 --- a/src/course-outline/unit-card/UnitCard.jsx +++ b/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]); diff --git a/src/course-outline/utils.jsx b/src/course-outline/utils.jsx index c946ae3577..3b5e87cc8a 100644 --- a/src/course-outline/utils.jsx +++ b/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 From f5a8afb8103dd15e637d32e977bd288c6f7879ce Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Sun, 28 Apr 2024 13:19:21 +0300 Subject: [PATCH 4/5] fix: Match focus border radius with button's --- src/search-modal/SearchModal.scss | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/search-modal/SearchModal.scss b/src/search-modal/SearchModal.scss index 228df4407c..94b31b0686 100644 --- a/src/search-modal/SearchModal.scss +++ b/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; + } } } From ecbaba89792ed2543b4dee01c57c80e31da5ab67 Mon Sep 17 00:00:00 2001 From: Yusuf Musleh Date: Tue, 30 Apr 2024 18:54:51 +0300 Subject: [PATCH 5/5] fix: Only expand (sub)sectionw with search result --- .../section-card/SectionCard.jsx | 46 ++++++- .../section-card/SectionCard.test.jsx | 129 ++++++++++++++---- .../subsection-card/SubsectionCard.jsx | 20 ++- .../subsection-card/SubsectionCard.test.jsx | 42 ++++-- 4 files changed, 198 insertions(+), 39 deletions(-) diff --git a/src/course-outline/section-card/SectionCard.jsx b/src/course-outline/section-card/SectionCard.jsx index 4eea3175d9..f8d11a4cd0 100644 --- a/src/course-outline/section-card/SectionCard.jsx +++ b/src/course-outline/section-card/SectionCard.jsx @@ -45,7 +45,33 @@ const SectionCard = ({ const [searchParams] = useSearchParams(); const locatorId = searchParams.get('show'); const isScrolledToElement = locatorId === section.id; - const [isExpanded, setIsExpanded] = useState(locatorId ? !!locatorId : isSectionsExpanded); + + // 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'; @@ -83,8 +109,8 @@ const SectionCard = ({ useEffect(() => { // If the locatorId is set/changed, we need to make sure that the section is expanded - // in order to scroll to search result, otherwise leave it as is. - setIsExpanded((prevState) => (locatorId ? !!locatorId : prevState)); + // if it contains the result, in order to scroll to it + setIsExpanded((prevState) => containsSearchResult() || prevState); }, [locatorId, setIsExpanded]); // re-create actions object for customizations @@ -261,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, diff --git a/src/course-outline/section-card/SectionCard.test.jsx b/src/course-outline/section-card/SectionCard.test.jsx index 5c99814c00..6f0a965ad9 100644 --- a/src/course-outline/section-card/SectionCard.test.jsx +++ b/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( - +const renderComponent = (props, entry = '/') => render( + - - - children - - + + + + children + + + , ); @@ -148,4 +195,38 @@ describe('', () => { 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(); + }); }); diff --git a/src/course-outline/subsection-card/SubsectionCard.jsx b/src/course-outline/subsection-card/SubsectionCard.jsx index b798fa5b99..9f134fdbe1 100644 --- a/src/course-outline/subsection-card/SubsectionCard.jsx +++ b/src/course-outline/subsection-card/SubsectionCard.jsx @@ -73,7 +73,14 @@ const SubsectionCard = ({ actions.allowMoveDown = !isEmpty(moveDownDetails); // Expand the subsection if a search result should be shown/scrolled to - const [isExpanded, setIsExpanded] = useState(locatorId ? !!locatorId : !isHeaderVisible); + 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, @@ -141,8 +148,8 @@ const SubsectionCard = ({ useEffect(() => { // If the locatorId is set/changed, we need to make sure that the subsection is expanded - // in order to scroll to search result - setIsExpanded((prevState) => (locatorId ? !!locatorId : prevState)); + // if it contains the result, in order to scroll to it + setIsExpanded((prevState) => (containsSearchResult() || prevState)); }, [locatorId, setIsExpanded]); useEffect(() => { @@ -273,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, diff --git a/src/course-outline/subsection-card/SubsectionCard.test.jsx b/src/course-outline/subsection-card/SubsectionCard.test.jsx index 167be766a9..08dfd4bb6e 100644 --- a/src/course-outline/subsection-card/SubsectionCard.test.jsx +++ b/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('', () => { 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(); + }); });