Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: redirect to unit page if the hit or its parent is a unit #957

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 0 additions & 15 deletions src/course-outline/CourseOutline.scss
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,3 @@
@import "./publish-modal/PublishModal";
@import "./drag-helper/SortableItem";
@import "./xblock-status/XBlockStatus";

div.row:has(> div > div.highlight) {
animation: 5s glow;
animation-timing-function: cubic-bezier(1, 0, .72, .04);
}

@keyframes glow {
0% {
box-shadow: 0 0 5px 5px $primary-500;
}

100% {
box-shadow: unset;
}
}
16 changes: 12 additions & 4 deletions src/course-unit/course-xblock/CourseXBlock.jsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import { useEffect, useRef } from 'react';
import PropTypes from 'prop-types';
import { useDispatch, useSelector } from 'react-redux';
import { useNavigate } from 'react-router-dom';
import {
ActionRow, Card, Dropdown, Icon, IconButton, useToggle,
} from '@openedx/paragon';
import { EditOutline as EditIcon, MoreVert as MoveVertIcon } from '@openedx/paragon/icons';
import { useIntl } from '@edx/frontend-platform/i18n';
import { useNavigate, useSearchParams } from 'react-router-dom';

import { getCanEdit, getCourseId } from 'CourseAuthoring/course-unit/data/selectors';
import DeleteModal from '../../generic/delete-modal/DeleteModal';
Expand All @@ -31,6 +31,10 @@ const CourseXBlock = ({
const courseId = useSelector(getCourseId);
const intl = useIntl();

const [searchParams] = useSearchParams();
const locatorId = searchParams.get('show');
const isScrolledToElement = locatorId === id;

const visibilityMessage = userPartitionInfo.selectedGroupsLabel
? intl.formatMessage(messages.visibilityMessage, { selectedGroupsLabel: userPartitionInfo.selectedGroupsLabel })
: null;
Expand Down Expand Up @@ -64,13 +68,17 @@ const CourseXBlock = ({

useEffect(() => {
// if this item has been newly added, scroll to it.
if (courseXBlockElementRef.current && shouldScroll) {
if (courseXBlockElementRef.current && (shouldScroll || isScrolledToElement)) {
scrollToElement(courseXBlockElementRef.current);
}
}, []);
}, [isScrolledToElement]);
Comment on lines +71 to +74
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, there is another way to do this without using react state at all.

  • Give the element a unique ID e.g. xblock-id-abcde
  • Set the url hash to the above ID, i.e. "...some/url/page#xblock-id-abcde"
  • Use the ":target" pseudo-class to apply the glow style.

If the URL fragment matches an ID the browser should automatically scroll to it, and it applies the :target pseudo-class ref.

This means by using this mechanism you can avoid any code for scrolling, and applying a class. Also changing this value doesn't cause the page to refresh since it's client-side only.

The only disadvantage is that this is never passed to the server. What that means is that the server can't perform any action based on this value. Here since there is no server this is a moot point, but worth mentioning anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome @xitij2000! Thank you for this.

Using targets here would be a better approach, but I think refactoring it (we already had some scrolling function feature) will put us out of schedule/scope.

But I think it is worth refactoring it next time we hit this feature.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly, that makes sense, and keeping this part of the JavaScript code also opens up other possibilities. No need for a refactoring already.


return (
<div ref={courseXBlockElementRef} {...props}>
<div
ref={courseXBlockElementRef}
{...props}
className={isScrolledToElement ? 'xblock-highlight' : undefined}
>
<Card className="mb-1">
<Card.Header
title={title}
Expand Down
14 changes: 12 additions & 2 deletions src/course-unit/hooks.jsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { useEffect, useState } from 'react';
import { useDispatch, useSelector } from 'react-redux';
import { useNavigate } from 'react-router-dom';
import { useNavigate, useSearchParams } from 'react-router-dom';

import { RequestStatus } from '../data/constants';
import {
Expand Down Expand Up @@ -31,6 +31,7 @@
// eslint-disable-next-line import/prefer-default-export
export const useCourseUnit = ({ courseId, blockId }) => {
const dispatch = useDispatch();
const [searchParams] = useSearchParams();

const [isErrorAlert, toggleErrorAlert] = useState(false);
const [hasInternetConnectionError, setInternetConnectionError] = useState(false);
Expand Down Expand Up @@ -84,7 +85,16 @@

const handleNavigate = (id) => {
if (sequenceId) {
navigate(`/course/${courseId}/container/${blockId}/${id}`, { replace: true });
const path = `/course/${courseId}/container/${blockId}/${id}`;
const options = { replace: true };
if (searchParams.size) {
navigate({

Check warning on line 91 in src/course-unit/hooks.jsx

View check run for this annotation

Codecov / codecov/patch

src/course-unit/hooks.jsx#L91

Added line #L91 was not covered by tests
pathname: path,
search: `?${searchParams}`,
}, options);
} else {
navigate(path, options);
}
}
};

Expand Down
5 changes: 5 additions & 0 deletions src/custom.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,8 @@ declare module '*.svg' {
const content: string;
export default content;
}

declare module '*.json' {
const value: any;
export default value;
}
22 changes: 22 additions & 0 deletions src/index.scss
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,25 @@
@import "search-modal/SearchModal";
@import "certificates/scss/Certificates";
@import "group-configurations/GroupConfigurations";

// To apply the glow effect to the selected Section/Subsection, in the Course Outline
div.row:has(> div > div.highlight) {
animation: 5s glow;
animation-timing-function: cubic-bezier(1, 0, .72, .04);
}

// To apply the glow effect to the selected xblock, in the Unit Outline
div.xblock-highlight {
animation: 5s glow;
animation-timing-function: cubic-bezier(1, 0, .72, .04);
}

@keyframes glow {
0% {
box-shadow: 0 0 5px 5px $primary-500;
}

100% {
box-shadow: unset;
}
}
114 changes: 96 additions & 18 deletions src/search-modal/SearchResult.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,81 @@ function getItemIcon(blockType) {
return STRUCTURAL_TYPE_ICONS[blockType] ?? COMPONENT_TYPE_ICON_MAP[blockType] ?? Article;
}

/**
* Returns the URL Suffix for library/library component hit
* @param {import('./data/api').ContentHit} hit
* @param {string} libraryAuthoringMfeUrl
* @returns string
*/
function getLibraryHitUrl(hit, libraryAuthoringMfeUrl) {
const { contextKey } = hit;
return `${libraryAuthoringMfeUrl}/library/${contextKey}`;
}

/**
* Returns the URL Suffix for a unit hit
* @param {import('./data/api').ContentHit} hit
* @returns string
*/
function getUnitUrlSuffix(hit) {
const { contextKey, usageKey } = hit;
return `course/${contextKey}/container/${usageKey}`;
}

/**
* Returns the URL Suffix for a unit component hit
* @param {import('./data/api').ContentHit} hit
* @returns string
*/
function getUnitComponentUrlSuffix(hit) {
const { breadcrumbs, contextKey, usageKey } = hit;
if (breadcrumbs.length > 1) {
const parent = breadcrumbs[breadcrumbs.length - 1];

if ('usageKey' in parent) {
return `course/${contextKey}/container/${parent.usageKey}?show=${encodeURIComponent(usageKey)}`;
}
}

// istanbul ignore next - This case should never be reached
return `course/${contextKey}`;
}

/**
* Returns the URL Suffix for a course component hit
* @param {import('./data/api').ContentHit} hit
* @returns string
*/
function getCourseComponentUrlSuffix(hit) {
const { contextKey, usageKey } = hit;
return `course/${contextKey}?show=${encodeURIComponent(usageKey)}`;
}

/**
* Returns the URL Suffix for the search hit param
* @param {import('./data/api').ContentHit} hit
* @returns string
*/
function getUrlSuffix(hit) {
const { blockType, breadcrumbs } = hit;

// Check if is a unit
if (blockType === 'vertical') {
return getUnitUrlSuffix(hit);
}

// Check if the parent is a unit
if (breadcrumbs.length > 1) {
const parent = breadcrumbs[breadcrumbs.length - 1];

if ('usageKey' in parent && parent.usageKey.includes('type@vertical')) {
return getUnitComponentUrlSuffix(hit);
}
}

return getCourseComponentUrlSuffix(hit);
}

/**
* A single search result (row), usually represents an XBlock/Component
* @type {React.FC<{hit: import('./data/api').ContentHit}>}
Expand All @@ -43,33 +118,34 @@ const SearchResult = ({ hit }) => {
const { closeSearchModal } = useSearchContext();
const { libraryAuthoringMfeUrl, redirectToLibraryAuthoringMfe } = useSelector(getStudioHomeData);

const { usageKey } = hit;

const noRedirectUrl = usageKey.startsWith('lb:') && !redirectToLibraryAuthoringMfe;

/**
* Returns the URL for the context of the hit
*/
const getContextUrl = React.useCallback((newWindow = false) => {
const { contextKey, usageKey } = hit;
const { contextKey } = hit;

if (contextKey.startsWith('course-v1:')) {
const courseSufix = `course/${contextKey}?show=${encodeURIComponent(usageKey)}`;
const urlSuffix = getUrlSuffix(hit);

if (newWindow) {
return `${getPath(getConfig().PUBLIC_PATH)}${courseSufix}`;
return `${getPath(getConfig().PUBLIC_PATH)}${urlSuffix}`;
}
return `/${courseSufix}`;
return `/${urlSuffix}`;
}

if (usageKey.startsWith('lb:')) {
if (redirectToLibraryAuthoringMfe) {
return `${libraryAuthoringMfeUrl}library/${contextKey}`;
return getLibraryHitUrl(hit, libraryAuthoringMfeUrl);
}
}

// No context URL for this hit
// No context URL for this hit (e.g. a library without library authoring mfe)
return undefined;
}, [libraryAuthoringMfeUrl, redirectToLibraryAuthoringMfe]);

const redirectUrl = React.useMemo(() => getContextUrl(), [libraryAuthoringMfeUrl, redirectToLibraryAuthoringMfe]);
const newWindowUrl = React.useMemo(
() => getContextUrl(true),
[libraryAuthoringMfeUrl, redirectToLibraryAuthoringMfe],
);
}, [libraryAuthoringMfeUrl, redirectToLibraryAuthoringMfe, hit]);

/**
* Opens the context of the hit in a new window
Expand All @@ -78,6 +154,7 @@ const SearchResult = ({ hit }) => {
*/
const openContextInNewWindow = (e) => {
e.stopPropagation();
const newWindowUrl = getContextUrl(true);
/* istanbul ignore next */
if (!newWindowUrl) {
return;
Expand All @@ -90,8 +167,9 @@ const SearchResult = ({ hit }) => {
* @param {(React.MouseEvent | React.KeyboardEvent)} e
* @returns {void}
*/
const navigateToContext = (e) => {
const navigateToContext = React.useCallback((e) => {
e.stopPropagation();
const redirectUrl = getContextUrl();

/* istanbul ignore next */
if (!redirectUrl) {
Expand All @@ -112,16 +190,16 @@ const SearchResult = ({ hit }) => {

navigate(redirectUrl);
closeSearchModal();
};
}, [getContextUrl]);

return (
<Stack
className={`border-bottom search-result p-2 align-items-start ${!redirectUrl ? 'text-muted' : ''}`}
className={`border-bottom search-result p-2 align-items-start ${noRedirectUrl ? 'text-muted' : ''}`}
direction="horizontal"
gap={3}
onClick={navigateToContext}
onKeyDown={navigateToContext}
tabIndex={redirectUrl ? 0 : undefined}
tabIndex={noRedirectUrl ? undefined : 0}
role="button"
>
<Icon className="text-muted" src={getItemIcon(hit.blockType)} />
Expand All @@ -140,7 +218,7 @@ const SearchResult = ({ hit }) => {
<IconButton
src={OpenInNew}
iconAs={Icon}
disabled={!newWindowUrl}
disabled={noRedirectUrl ? true : undefined}
onClick={openContextInNewWindow}
alt={intl.formatMessage(messages.openInNewWindow)}
/>
Expand Down
Loading
Loading