-
Notifications
You must be signed in to change notification settings - Fork 8
Introduce courseNavigationBar on header slot #106
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
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| import { useNavigate, useLocation } from 'react-router-dom'; | ||
| import classNames from 'classnames'; | ||
| import { useQuery } from '@tanstack/react-query'; | ||
| import { Tab, Tabs } from '@openedx/paragon'; | ||
| import { Slot, useIntl } from '../../../runtime'; | ||
| import { getCourseHomeCourseMetadata } from './data/service'; | ||
| import messages from './messages'; | ||
| import './course-tabs-navigation.scss'; | ||
|
|
||
| interface CourseMetaData { | ||
| tabs: { | ||
| title: string, | ||
| slug: string, | ||
| url: string, | ||
| }[], | ||
| isMasquerading: boolean, | ||
| } | ||
|
|
||
| const extractCourseId = (pathname: string): string => { | ||
| const courseRegex = /\/courses?\/([^/]+)/; | ||
| const courseMatch = courseRegex.exec(pathname); | ||
| return courseMatch ? courseMatch[1] : ''; | ||
| }; | ||
|
|
||
| const CourseTabsNavigation = () => { | ||
| const location = useLocation(); | ||
| const intl = useIntl(); | ||
| const navigate = useNavigate(); | ||
|
|
||
| const courseId = extractCourseId(location.pathname); | ||
|
|
||
| const { data } = useQuery({ | ||
| queryKey: ['org.openedx.frontend.app.header.course-meta', courseId], | ||
|
Contributor
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. shouldn't this base ID be a constant? |
||
| queryFn: () => getCourseHomeCourseMetadata(courseId), | ||
| retry: 2, | ||
diana-villalvazo-wgu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| enabled: !!courseId, | ||
| }); | ||
|
|
||
| if (!courseId) { | ||
| return null; | ||
| } | ||
|
|
||
| const { tabs = [] }: CourseMetaData = data ?? {}; | ||
|
|
||
| const handleSelectedTab = (eventKey: string | null) => { | ||
| const selectedUrl = tabs.find(tab => tab.slug === eventKey)?.url ?? '/'; | ||
|
|
||
| try { | ||
| if (selectedUrl.startsWith('http://') || selectedUrl.startsWith('https://')) { | ||
|
Contributor
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. Just to understand in which case the selectedUrl woudln't have http or https?
Author
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. a case could be a custom implementation that just points to a route on same domain for example if someone sent |
||
| const url = new URL(selectedUrl); | ||
| if (url.origin === window.location.origin) { | ||
| navigate(url.pathname + url.search + url.hash); | ||
|
Author
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. tried to use navigate as much as possible so we can preserve app state and avoid unneeded full page load 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. I see, thanks for the explanation |
||
| } else { | ||
| window.location.href = selectedUrl; | ||
| } | ||
| } else { | ||
| navigate(selectedUrl); | ||
|
Contributor
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. if there's an issue with the URL you are trying to parse why are you navigating to it? |
||
| } | ||
| } catch (error) { | ||
| navigate(selectedUrl); | ||
| } | ||
| }; | ||
|
|
||
| return ( | ||
| <div id="courseTabsNavigation" className={classNames('course-tabs-navigation')}> | ||
|
Contributor
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. Any particular reason we need |
||
| <div className="container-xl"> | ||
| <div className="nav-bar"> | ||
| <div className="nav-menu"> | ||
| <Tabs className="nav-underline-tabs" aria-label={intl.formatMessage(messages.courseMaterial)} onSelect={handleSelectedTab}> | ||
| {tabs.map(({ title, slug }) => ( | ||
| <Tab eventKey={slug} title={title} key={slug} /> | ||
|
Contributor
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. I think we might want to use |
||
| ))} | ||
| </Tabs> | ||
| </div> | ||
| <Slot id="org.openedx.frontend.slot.header.courseNavigationBar.extraContent.v1" /> | ||
|
Author
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. This slot was added because on current learning MFE this nav bar has some extra content related to |
||
| </div> | ||
| </div> | ||
| </div> | ||
| ); | ||
| }; | ||
|
|
||
| export default CourseTabsNavigation; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| export const activeRolesForCourseNavigationBar = [ | ||
| 'org.openedx.frontend.role.learning', | ||
| 'org.openedx.frontend.role.discussions', | ||
| 'org.openedx.frontend.role.instructor', | ||
| ]; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| .course-tabs-navigation { | ||
| border-bottom: 2px solid rgb(232.5, 229.5, 228); // var(--pgn-color-nav-tabs-base-border-base) | ||
|
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. why do you remove the variable for a hardcoded color? maybe you could create a variable something like
Author
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. when i created this PR, design tokens on frontend base wasn't ready, so didn't work, i left the comment to remember that and update it when that was ready |
||
|
|
||
| .nav-tabs { | ||
| border-bottom: none; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| import { getSiteConfig, getAuthenticatedHttpClient, camelCaseObject } from '../../../../runtime'; | ||
|
|
||
| export const getCourseMetadataApiUrl = (courseId) => `${getSiteConfig().lmsBaseUrl}/api/course_home/course_metadata/${courseId}`; | ||
|
|
||
| function normalizeCourseHomeCourseMetadata(metadata) { | ||
| const data = camelCaseObject(metadata); | ||
| return { | ||
| ...data, | ||
| tabs: (data.tabs || []).map(tab => ({ | ||
| slug: tab.tabId === 'courseware' ? 'outline' : tab.tabId, | ||
| title: tab.title, | ||
| url: tab.url, | ||
| })), | ||
| isMasquerading: data.originalUserIsStaff && !data.isStaff, | ||
| }; | ||
| } | ||
|
|
||
| export async function getCourseHomeCourseMetadata(courseId) { | ||
| const url = getCourseMetadataApiUrl(courseId); | ||
diana-villalvazo-wgu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| const { data } = await getAuthenticatedHttpClient().get(url); | ||
|
|
||
| return normalizeCourseHomeCourseMetadata(data); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| import { defineMessages } from '../../../runtime'; | ||
|
|
||
| const messages = defineMessages({ | ||
| courseMaterial: { | ||
| id: 'org.openedx.frontend.slot.header.courseNavigationBar.tabs.label', | ||
| defaultMessage: 'Course Material', | ||
| description: 'The accessible label for course tabs navigation', | ||
| }, | ||
| }); | ||
|
|
||
| export default messages; |
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.
if possible make it absolute