refactor: Use the same mechanism for autogenerated component id (patt…

…ernfly#830) To make the code more consistent, I added a method getUniqueId that is used by all components that need a unique id. I put this id recovery in the defaultProps object. Add also some tests for util file.
redallen · Oct 26, 2018 · b8bc4b1 · b8bc4b1
1 parent c7072c8
commit b8bc4b1
Show file tree

Hide file tree

Showing 6 changed files with 63 additions and 47 deletions.
diff --git a/packages/patternfly-4/react-core/src/components/Nav/NavExpandable.js b/packages/patternfly-4/react-core/src/components/Nav/NavExpandable.js
@@ -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 */
@@ -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>
@@ -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()}
@@ -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>
                   )}

diff --git a/packages/patternfly-4/react-core/src/components/Nav/NavGroup.js b/packages/patternfly-4/react-core/src/components/Nav/NavGroup.js
@@ -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 */
@@ -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>

diff --git a/packages/patternfly-4/react-core/src/components/Nav/__snapshots__/Nav.test.js.snap b/packages/patternfly-4/react-core/src/components/Nav/__snapshots__/Nav.test.js.snap
@@ -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
@@ -613,7 +612,6 @@ exports[`Expandable Nav List 1`] = `
           >
             <li
               className="pf-c-nav__item"
-              id="grp-1"
               onClick={[Function]}
             >
               <a
@@ -856,7 +854,6 @@ exports[`Expandable Nav List with aria label 1`] = `
           >
             <li
               className="pf-c-nav__item"
-              id="grp-1"
               onClick={[Function]}
             >
               <a
@@ -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"
@@ -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"

diff --git a/packages/patternfly-4/react-core/src/components/Progress/Progress.js b/packages/patternfly-4/react-core/src/components/Progress/Progress.js
@@ -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',
@@ -49,23 +50,26 @@ const defaultProps = {
 };
 
 class Progress extends Component {
-  constructor(props) {
-    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;
@@ -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}

diff --git a/packages/patternfly-4/react-core/src/internal/util.js b/packages/patternfly-4/react-core/src/internal/util.js
@@ -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}`;
+}
diff --git a/packages/patternfly-4/react-core/src/internal/util.test.js b/packages/patternfly-4/react-core/src/internal/util.test.js
@@ -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-');
+});