-
Notifications
You must be signed in to change notification settings - Fork 80
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
Changes from 7 commits
9901d9b
1bbca0a
949b3c6
78dc0c4
f873a98
f04e43d
a13ccb5
0493375
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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'; | ||
|
@@ -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; | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
If the URL fragment matches an ID the browser should automatically scroll to it, and it applies the 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This exact same animation already exists in
CourseOutline.scss
, can you boost it to a more common location and reuse the code instead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the suggestion @xitij2000! I moved it here: 0493375