Skip to content

refactor: Use the same mechanism for autogenerated component id #830

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
tlabaj merged 1 commit into from
Oct 26, 2018
Merged

refactor: Use the same mechanism for autogenerated component id #830

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
30 changes: 16 additions & 14 deletions packages/patternfly-4/react-core/src/components/Nav/NavExpandable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import PropTypes from 'prop-types';
import NavToggle from './NavToggle';
import { AngleRightIcon } from '@patternfly/react-icons';
import { NavContext } from './Nav';
import { getUniqueId } from '../../internal/util';

const propTypes = {
/** Title shown for the expandable list */
Expand Down Expand Up @@ -37,19 +38,20 @@ const defaultProps = {
};

class NavExpandable extends React.Component {
constructor(props) {
super(props);

this.uniqueId =
props.id ||
new Date().getTime() +
Math.random()
.toString(36)
.slice(2);
}
id = this.props.id || getUniqueId();

render() {
const { title, srText, isExpanded: defaultExpanded, children, className, groupId, isActive, ...props } = this.props;
const {
id,
title,
srText,
isExpanded: defaultExpanded,
children,
className,
groupId,
isActive,
...props
} = this.props;

return (
<NavContext.Consumer>
Expand All @@ -68,7 +70,7 @@ class NavExpandable extends React.Component {
>
<a
className={css(styles.navLink)}
id={srText ? null : this.uniqueId}
id={srText ? null : this.id}
href="#"
onClick={e => e.preventDefault()}
onMouseDown={e => e.preventDefault()}
Expand All @@ -81,11 +83,11 @@ class NavExpandable extends React.Component {
</a>
<section
className={css(styles.navSubnav)}
aria-labelledby={this.uniqueId}
aria-labelledby={this.id}
hidden={isExpanded ? null : true}
>
{srText && (
<h2 className={css(a11yStyles.srOnly)} id={this.uniqueId}>
<h2 className={css(a11yStyles.srOnly)} id={this.id}>
{srText}
</h2>
)}
Expand Down
18 changes: 5 additions & 13 deletions packages/patternfly-4/react-core/src/components/Nav/NavGroup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React from 'react';
import styles from '@patternfly/patternfly-next/components/Nav/nav.css';
import { css } from '@patternfly/react-styles';
import PropTypes from 'prop-types';
import { getUniqueId } from '../../internal/util';

const propTypes = {
/** Title shown for the group */
Expand All @@ -21,23 +22,14 @@ const defaultProps = {
};

class NavGroup extends React.Component {
constructor(props) {
super(props);

this.uniqueId =
props.id ||
new Date().getTime() +
Math.random()
.toString(36)
.slice(2);
}
id = this.props.id || getUniqueId();

render() {
const { title, children, className, ...props } = this.props;
const { id, title, children, className, ...props } = this.props;

return (
<section className={css(styles.navSection, className)} aria-labelledby={this.uniqueId} {...props}>
<h2 className={css(styles.navSectionTitle)} id={this.uniqueId}>
<section className={css(styles.navSection, className)} aria-labelledby={this.id} {...props}>
<h2 className={css(styles.navSectionTitle)} id={this.id}>
{title}
</h2>
<ul className={css(styles.navSimpleList)}>{children}</ul>
Expand Down
5 changes: 0 additions & 5 deletions packages/patternfly-4/react-core/src/components/Nav/__snapshots__/Nav.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,6 @@ exports[`Expandable Nav List - Trigger toggle 1`] = `
>
<li
className="pf-c-nav__item expandable-group"
id="grp-1"
onClick={[Function]}
>
<a
Expand Down Expand Up @@ -613,7 +612,6 @@ exports[`Expandable Nav List 1`] = `
>
<li
className="pf-c-nav__item"
id="grp-1"
onClick={[Function]}
>
<a
Expand Down Expand Up @@ -856,7 +854,6 @@ exports[`Expandable Nav List with aria label 1`] = `
>
<li
className="pf-c-nav__item"
id="grp-1"
onClick={[Function]}
>
<a
Expand Down Expand Up @@ -1251,7 +1248,6 @@ exports[`Nav Grouped List 1`] = `
<section
aria-labelledby="grp-1"
className="pf-c-nav__section"
id="grp-1"
>
<h2
className="pf-c-nav__section-title"
Expand Down Expand Up @@ -1366,7 +1362,6 @@ exports[`Nav Grouped List 1`] = `
<section
aria-labelledby="grp-2"
className="pf-c-nav__section"
id="grp-2"
>
<h2
className="pf-c-nav__section-title"
Expand Down
34 changes: 19 additions & 15 deletions packages/patternfly-4/react-core/src/components/Progress/Progress.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import styles from '@patternfly/patternfly-next/components/Progress/progress.css
import { css, getModifier } from '@patternfly/react-styles';
import PropTypes from 'prop-types';
import ProgressContainer, { ProgressMeasureLocation, ProgressVariant } from './ProgressContainer';
import { getUniqueId } from '../../internal/util';

export const ProgressSize = {
sm: 'sm',
Expand Down Expand Up @@ -49,23 +50,26 @@ const defaultProps = {
};

class Progress extends Component {
constructor(props) {
Copy link
Collaborator

@jschuler jschuler Oct 25, 2018

Choose a reason for hiding this comment

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

Great PR! But we do need to initialize the unique id in the constructors. Or at least have some other way of assigning unique id to the component. Props get evaluated before the component instances are created so if you have multiple of the same components on the page they will all have the same id otherwise.

super(props);
this.state = {
uniqueId:
props.id ||
`progress-${new Date().getTime()}${Math.random()
.toString(36)
.slice(2)}`
};
}
id = this.props.id || getUniqueId();

render() {
const { className, size, id, value, title, label, variant, measureLocation, min, max, valueText, ...props } = this.props;
const { uniqueId } = this.state;
const {
id,
className,
size,
value,
title,
label,
variant,
measureLocation,
min,
max,
valueText,
...props
} = this.props;
const additionalProps = {
...props,
...(valueText ? { 'aria-valuetext': valueText } : { 'aria-describedby': `${id}-description` })
...(valueText ? { 'aria-valuetext': valueText } : { 'aria-describedby': `${this.id}-description` })
};
let limitedValue;
limitedValue = value < min ? min : value;
Expand All @@ -80,14 +84,14 @@ class Progress extends Component {
getModifier(styles, measureLocation === ProgressMeasureLocation.inside ? ProgressSize.lg : size, ''),
className
)}
id={uniqueId}
id={this.id}
role="progressbar"
aria-valuemin={min}
aria-valuenow={limitedValue}
aria-valuemax={max}
>
<ProgressContainer
parentId={uniqueId}
parentId={this.id}
value={limitedValue}
title={title}
label={label}
Expand Down
9 changes: 9 additions & 0 deletions packages/patternfly-4/react-core/src/internal/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,12 @@
export function capitalize(input) {
return input[0].toUpperCase() + input.substring(1);
}

export function getUniqueId(prefix = 'pf') {
const uid =
new Date().getTime() +
Math.random()
.toString(36)
.slice(2);
return `${prefix}-${uid}`;
}
14 changes: 14 additions & 0 deletions packages/patternfly-4/react-core/src/internal/util.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { capitalize, getUniqueId } from './util';

test('capitalize', () => {
expect(capitalize('foo')).toBe('Foo');
});

test('getUniqueId', () => {
expect(getUniqueId()).not.toBe(getUniqueId());
});

test('getUniqueId prefixed', () => {
expect(getUniqueId().substring(0, 3)).toBe('pf-');
expect(getUniqueId('pf-switch').substring(0, 10)).toBe('pf-switch-');
});