Skip to content

feat: Header - Focus order meaning - Open Edx Demo Course #30

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jun 16, 2025
Merged

feat: Header - Focus order meaning - Open Edx Demo Course #30

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
52f7bd0
feat: Header - Focus order meaning - Open Edx Demo Course
arbirali May 19, 2025
4dec464
refactor: unify responsive rendering using existing Responsive component
arbirali Jun 5, 2025
95db69a
test: update test to handle conditional rendering logic
arbirali Jun 5, 2025
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
22 changes: 16 additions & 6 deletions src/learning-header/LearningHeader.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import React, { useContext } from 'react';
import Responsive from 'react-responsive';
import PropTypes from 'prop-types';
import { getConfig } from '@edx/frontend-platform';
import { injectIntl, intlShape } from '@edx/frontend-platform/i18n';
Expand Down Expand Up @@ -30,6 +31,13 @@ const LearningHeader = ({
<header className="learning-header customise indigo-header-version">
<a className="sr-only sr-only-focusable" href="#main-content">{intl.formatMessage(messages.skipNavLink)}</a>
<div className="container-xl py-2 d-flex align-items-center">
{showUserDropdown && authenticatedUser && (
<Responsive maxWidth={991}>
<AuthenticatedUserDropdown
username={authenticatedUser.username}
/>
</Responsive>
)}
Comment on lines +34 to +40
Copy link

Choose a reason for hiding this comment

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

How is this different from the same code below? Why is this added?

Copy link
Author

Choose a reason for hiding this comment

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

@DawoudSheraz This change adjusts the dropdown's DOM position based on screen size—placing it at the end on desktop and the start in mobile’s hamburger menu. This ensures proper focus order and prevents hidden content issues during keyboard navigation. Screenshots are also added in the PR description to demonstrate the fixed behavior across devices.

Copy link

Choose a reason for hiding this comment

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

There are no screenshots in PR description. I understand the context, but please add the screenshot as they are helpful for reviewers and anyone looking at PR.

Copy link
Author

Choose a reason for hiding this comment

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

@DawoudSheraz, I have attached the screenshots.

{headerLogo}
<div className="flex-grow-1 course-title-lockup d-flex" style={{ lineHeight: 1 }}>
<CourseInfoSlot courseOrg={courseOrg} courseNumber={courseNumber} courseTitle={courseTitle} />
Expand All @@ -46,12 +54,14 @@ const LearningHeader = ({
</div>
<ThemeToggleButton />
{showUserDropdown && authenticatedUser && (
<>
<LearningHelpSlot />
<AuthenticatedUserDropdown
username={authenticatedUser.username}
/>
</>
<>
<LearningHelpSlot />
<Responsive minWidth={992}>
<AuthenticatedUserDropdown
username={authenticatedUser.username}
/>
</Responsive>
</>
)}
{showUserDropdown && !authenticatedUser && (
<AnonymousUserMenu />
Expand Down
10 changes: 10 additions & 0 deletions src/learning-header/LearningHeader.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,16 @@ import {
} from '../setupTest';
import { LearningHeader as Header } from '../index';

jest.mock('react-responsive', () => ({
__esModule: true,
default: ({ minWidth, maxWidth, children }) => {
const screenWidth = 1200; // Simulate desktop
const matchesMin = minWidth !== undefined ? screenWidth >= minWidth : true;
const matchesMax = maxWidth !== undefined ? screenWidth <= maxWidth : true;
return matchesMin && matchesMax ? children : null;
},
}));

describe('Header', () => {
beforeAll(async () => {
// We need to mock AuthService to implicitly use `getAuthenticatedUser` within `AppContext.Provider`.
Expand Down