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.

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

Jump to bottom

feat(JumpLinks): consumed Penta updates #10027

Merged
merged 4 commits into from
Feb 13, 2024
Merged

feat(JumpLinks): consumed Penta updates #10027

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1da394d
feat(JumpLinks): consumed Penta updates
thatblindgeye Jan 19, 2024
4a94da0
Added button component instead of button classes
thatblindgeye Feb 1, 2024
8129c1a
Updated onClick typing in JumpLinks
thatblindgeye Feb 1, 2024
fb71836
Add type assertion for currentTarget in JumpLinks
thatblindgeye Feb 1, 2024
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
4 changes: 2 additions & 2 deletions packages/react-core/src/components/JumpLinks/JumpLinks.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ export const JumpLinks: React.FunctionComponent<JumpLinksProps> = ({
const itemIndex = jumpLinkIndex++;
const scrollItem = scrollItems[itemIndex];
return React.cloneElement(child as React.ReactElement<JumpLinksItemProps>, {
onClick(ev: React.MouseEvent<HTMLAnchorElement>) {
onClick(ev: React.MouseEvent) {
isLinkClicked.current = true;
// Items might have rendered after this component. Do a quick refresh.
let newScrollItems;
Expand Down Expand Up @@ -193,7 +193,7 @@ export const JumpLinks: React.FunctionComponent<JumpLinksProps> = ({
scrollableElement.scrollTo(0, newScrollItem.offsetTop - offset);
}
newScrollItem.focus();
window.history.pushState('', '', ev.currentTarget.href);
window.history.pushState('', '', (ev.currentTarget as HTMLAnchorElement).href);
ev.preventDefault();
setActiveIndex(itemIndex);
}
Expand Down
13 changes: 8 additions & 5 deletions packages/react-core/src/components/JumpLinks/JumpLinksItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,19 @@ import * as React from 'react';
import { css } from '@patternfly/react-styles';
import styles from '@patternfly/react-styles/css/components/JumpLinks/jump-links';
import { JumpLinksList } from './JumpLinksList';
import { Button, ButtonVariant } from '../Button';

export interface JumpLinksItemProps extends Omit<React.HTMLProps<HTMLLIElement>, 'onClick'> {
/** Whether this item is active. Parent JumpLinks component sets this when passed a `scrollableSelector`. */
isActive?: boolean;
/** Href for this link */
href?: string;
href: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This needs to be required, as without it the anchor internal to the component cannot receive keyboard focus.

If the intent is that a consumer can pass either an href to link to an ID on the page, or the onClick to handle this functionality themselves, then we would need to render either an anchor with an href or a button with the onClick, not an anchor with either/both.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make a codemod to at least warn consumers about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

patternfly/pf-codemods#559

/** Selector or HTMLElement to spy on */
node?: string | HTMLElement;
/** Text to be rendered inside span */
children?: React.ReactNode;
/** Click handler for anchor tag. Parent JumpLinks components tap into this. */
onClick?: (ev: React.MouseEvent<HTMLAnchorElement>) => void;
onClick?: (ev: React.MouseEvent) => void;
/** Class to add to li */
className?: string;
}
Expand All @@ -38,9 +39,11 @@ export const JumpLinksItem: React.FunctionComponent<JumpLinksItemProps> = ({
{...(isActive && { 'aria-current': 'location' })}
{...props}
>
<a className={styles.jumpLinksLink} href={href} onClick={onClick}>
<span className={styles.jumpLinksLinkText}>{children}</span>
</a>
<span className={styles.jumpLinksLink}>
<Button variant={ButtonVariant.link} component="a" href={href} onClick={onClick}>
<span className={styles.jumpLinksLinkText}>{children}</span>
</Button>
</span>
{sublists}
</li>
);
Expand Down
48 changes: 29 additions & 19 deletions packages/react-core/src/components/JumpLinks/__tests__/JumpLinks.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@ import { JumpLinksList } from '../JumpLinksList';
test('simple jumplinks', () => {
const { asFragment } = render(
<JumpLinks>
<JumpLinksItem>Inactive section</JumpLinksItem>
<JumpLinksItem isActive>Active section</JumpLinksItem>
<JumpLinksItem>Inactive section</JumpLinksItem>
<JumpLinksItem href="#">Inactive section</JumpLinksItem>
<JumpLinksItem href="#" isActive>
Active section
</JumpLinksItem>
<JumpLinksItem href="#">Inactive section</JumpLinksItem>
</JumpLinks>
);
expect(asFragment()).toMatchSnapshot();
Expand All @@ -18,9 +20,11 @@ test('simple jumplinks', () => {
test('jumplinks centered', () => {
const { asFragment } = render(
<JumpLinks isCentered>
<JumpLinksItem>Inactive section</JumpLinksItem>
<JumpLinksItem isActive>Active section</JumpLinksItem>
<JumpLinksItem>Inactive section</JumpLinksItem>
<JumpLinksItem href="#">Inactive section</JumpLinksItem>
<JumpLinksItem href="#" isActive>
Active section
</JumpLinksItem>
<JumpLinksItem href="#">Inactive section</JumpLinksItem>
</JumpLinks>
);
expect(asFragment()).toMatchSnapshot();
Expand All @@ -29,9 +33,11 @@ test('jumplinks centered', () => {
test('jumplinks with label', () => {
const { asFragment } = render(
<JumpLinks isCentered label="Jump to section">
<JumpLinksItem>Inactive section</JumpLinksItem>
<JumpLinksItem isActive>Active section</JumpLinksItem>
<JumpLinksItem>Inactive section</JumpLinksItem>
<JumpLinksItem href="#">Inactive section</JumpLinksItem>
<JumpLinksItem href="#" isActive>
Active section
</JumpLinksItem>
<JumpLinksItem href="#">Inactive section</JumpLinksItem>
</JumpLinks>
);
expect(asFragment()).toMatchSnapshot();
Expand All @@ -40,9 +46,11 @@ test('jumplinks with label', () => {
test('vertical with label', () => {
const { asFragment } = render(
<JumpLinks isVertical alwaysShowLabel label="Jump to section">
<JumpLinksItem>Inactive section</JumpLinksItem>
<JumpLinksItem isActive>Active section</JumpLinksItem>
<JumpLinksItem>Inactive section</JumpLinksItem>
<JumpLinksItem href="#">Inactive section</JumpLinksItem>
<JumpLinksItem href="#" isActive>
Active section
</JumpLinksItem>
<JumpLinksItem href="#">Inactive section</JumpLinksItem>
</JumpLinks>
);
expect(asFragment()).toMatchSnapshot();
Expand All @@ -51,17 +59,19 @@ test('vertical with label', () => {
test('expandable vertical with subsection', () => {
const { asFragment } = render(
<JumpLinks isVertical label="Jump to section" expandable={{ default: 'expandable' }}>
<JumpLinksItem>Inactive section</JumpLinksItem>
<JumpLinksItem>
<JumpLinksItem href="#">Inactive section</JumpLinksItem>
<JumpLinksItem href="#">
Section with active subsection
<JumpLinksList>
<JumpLinksItem isActive>Active subsection</JumpLinksItem>
<JumpLinksItem>Inactive subsection</JumpLinksItem>
<JumpLinksItem>Inactive subsection</JumpLinksItem>
<JumpLinksItem href="#" isActive>
Active subsection
</JumpLinksItem>
<JumpLinksItem href="#">Inactive subsection</JumpLinksItem>
<JumpLinksItem href="#">Inactive subsection</JumpLinksItem>
</JumpLinksList>
</JumpLinksItem>
<JumpLinksItem>Inactive section</JumpLinksItem>
<JumpLinksItem>Inactive section</JumpLinksItem>
<JumpLinksItem href="#">Inactive section</JumpLinksItem>
<JumpLinksItem href="#">Inactive section</JumpLinksItem>
</JumpLinks>
);
expect(asFragment()).toMatchSnapshot();
Expand Down
Loading
Loading