Changed Button to Link, and removed unnecessary props

- React Button was only ever used to render a link (a element)
- This creates a potential accessibility issue if developers want to render a button, it's confusing and incorrect
- Additionally, the unused prop (dialogTrigger) & aria-haspopup is non-compliant and has been removed

.eslintrc.js

@@ -70,7 +70,6 @@ module.exports = {
         'jsx-a11y/click-events-have-key-events': 'off',
         'jsx-a11y/interactive-supports-focus': 'off',
         'jsx-a11y/no-noninteractive-element-interactions': 'off',
-        'jsx-a11y/role-supports-aria-props': 'off',
         'no-restricted-syntax': 'off',
         'react-hooks/exhaustive-deps': 'off',
         'react-hooks/rules-of-hooks': 'off',

CHANGELOG.txt

@@ -14,6 +14,7 @@ Changelog
  * Maintenance: Remove duplicate 'path' in default_exclude_fields_in_copy (Ramchandra Shahi Thakuri)
  * Maintenance: Update unit tests to always use the faster, built in `html.parser` & remove `html5lib` dependency (Jake Howard)
  * Maintenance: Adjust Eslint rules for TypeScript files (Karthik Ayangar)
+    * Maintenance: Rename the React `Button` that only renders links (a element) to `Link` and remove unused prop & behavior that was non-compliant for aria role usage (Advik Kabra)
 
 
 6.0 (xx.xx.xxxx) - IN DEVELOPMENT

CONTRIBUTORS.md

@@ -793,6 +793,7 @@
 * Nix Asteri
 * Karthik Ayangar
 * mirusu400
+* Advik Kabra
 
 ## Translators
 

client/src/components/Button/Button.test.js → client/src/components/Link/Link.test.js

@@ -1,40 +1,37 @@
+/* eslint-disable jsx-a11y/anchor-is-valid */
 import React from 'react';
 import { shallow } from 'enzyme';
 
-import Button from './Button';
+import Link from './Link';
 
-describe('Button', () => {
+describe('Link', () => {
   it('exists', () => {
-    expect(Button).toBeDefined();
+    expect(Link).toBeDefined();
   });
 
   it('basic', () => {
-    expect(shallow(<Button />)).toMatchSnapshot();
+    expect(shallow(<Link />)).toMatchSnapshot();
   });
 
   it('#children', () => {
-    expect(shallow(<Button>To infinity and beyond!</Button>)).toMatchSnapshot();
+    expect(shallow(<Link>To infinity and beyond!</Link>)).toMatchSnapshot();
   });
 
   it('#accessibleLabel', () => {
     expect(
-      shallow(<Button accessibleLabel="I am here in the shadows" />),
+      shallow(<Link accessibleLabel="I am here in the shadows" />),
     ).toMatchSnapshot();
   });
 
-  it('#dialogTrigger', () => {
-    expect(shallow(<Button dialogTrigger />)).toMatchSnapshot();
-  });
-
   it('#target', () => {
     expect(
-      shallow(<Button target="_blank" rel="noreferrer" />),
+      shallow(<Link target="_blank" rel="noreferrer" />),
     ).toMatchSnapshot();
   });
 
   it('is clickable', () => {
     const onClick = jest.fn();
-    shallow(<Button onClick={onClick} />).simulate('click', {
+    shallow(<Link onClick={onClick} />).simulate('click', {
       preventDefault: jest.fn(),
       stopPropagation: jest.fn(),
     });
@@ -44,7 +41,7 @@ describe('Button', () => {
   it('dismisses click with no href', () => {
     // If no href is set, it should prevent default
     const preventDefault = jest.fn();
-    shallow(<Button />).simulate('click', {
+    shallow(<Link />).simulate('click', {
       preventDefault,
       stopPropagation: jest.fn(),
     });
@@ -53,7 +50,7 @@ describe('Button', () => {
 
   it('does not dismiss click if href is set', () => {
     const preventDefault = jest.fn();
-    shallow(<Button href="/admin/" />).simulate('click', {
+    shallow(<Link href="/admin/" />).simulate('click', {
       preventDefault,
       stopPropagation: jest.fn(),
     });
@@ -65,7 +62,7 @@ describe('Button', () => {
     const preventDefault = jest.fn();
     const navigate = jest.fn();
 
-    shallow(<Button href="/admin/" navigate={navigate} />).simulate('click', {
+    shallow(<Link href="/admin/" navigate={navigate} />).simulate('click', {
       preventDefault,
       stopPropagation: jest.fn(),
     });

client/src/components/Button/Button.tsx → client/src/components/Link/Link.tsx

@@ -28,21 +28,20 @@ const handleClick = (
   }
 };
 
-interface ButtonProps {
+interface LinkProps {
   className?: string;
   accessibleLabel?: string;
   href?: string;
   target?: string;
   preventDefault?: boolean;
   onClick?(e: React.MouseEvent): void;
-  dialogTrigger?: boolean;
   navigate?(url: string): Promise<void>;
 }
 
 /**
  * A reusable button. Uses a <a> tag underneath.
  */
-const Button: React.FunctionComponent<ButtonProps> = ({
+const Link: React.FunctionComponent<LinkProps> = ({
   className = '',
   children,
   accessibleLabel,
@@ -50,7 +49,6 @@ const Button: React.FunctionComponent<ButtonProps> = ({
   target,
   preventDefault = true,
   onClick,
-  dialogTrigger,
   navigate,
 }) => {
   const hasText = React.Children.count(children) > 0;
@@ -65,11 +63,10 @@ const Button: React.FunctionComponent<ButtonProps> = ({
       rel={target === '_blank' ? 'noreferrer' : undefined}
       href={href}
       target={target}
-      aria-haspopup={dialogTrigger ? 'dialog' : undefined}
     >
       {hasText ? children : accessibleElt}
     </a>
   );
 };
 
-export default Button;
+export default Link;

client/src/components/Button/__snapshots__/Button.test.js.snap → client/src/components/Link/__snapshots__/Link.test.js.snap

@@ -1,6 +1,6 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
-exports[`Button #accessibleLabel 1`] = `
+exports[`Link #accessibleLabel 1`] = `
 <a
   className=""
   href="#"
@@ -14,7 +14,7 @@ exports[`Button #accessibleLabel 1`] = `
 </a>
 `;
 
-exports[`Button #children 1`] = `
+exports[`Link #children 1`] = `
 <a
   className=""
   href="#"
@@ -24,16 +24,7 @@ exports[`Button #children 1`] = `
 </a>
 `;
 
-exports[`Button #dialogTrigger 1`] = `
-<a
-  aria-haspopup="dialog"
-  className=""
-  href="#"
-  onClick={[Function]}
-/>
-`;
-
-exports[`Button #target 1`] = `
+exports[`Link #target 1`] = `
 <a
   className=""
   href="#"
@@ -43,7 +34,7 @@ exports[`Button #target 1`] = `
 />
 `;
 
-exports[`Button basic 1`] = `
+exports[`Link basic 1`] = `
 <a
   className=""
   href="#"

client/src/components/PageExplorer/PageExplorerHeader.test.js

@@ -46,7 +46,7 @@ describe('PageExplorerHeader', () => {
 
   it('#onClick', () => {
     const wrapper = mount(<PageExplorerHeader {...mockProps} />);
-    wrapper.find('Button').simulate('click');
+    wrapper.find('Link').simulate('click');
 
     expect(mockProps.onClick).toHaveBeenCalledTimes(1);
   });

client/src/components/PageExplorer/PageExplorerHeader.tsx

@@ -2,7 +2,7 @@ import React from 'react';
 import { ADMIN_URLS } from '../../config/wagtailConfig';
 
 import { gettext } from '../../utils/gettext';
-import Button from '../Button/Button';
+import Link from '../Link/Link';
 import Icon from '../Icon/Icon';
 import { PageState } from './reducers/nodes';
 
@@ -68,7 +68,7 @@ const PageExplorerHeader: React.FunctionComponent<PageExplorerHeaderProps> = ({
 
   return (
     <div className="c-page-explorer__header">
-      <Button
+      <Link
         href={!isSiteRoot ? `${ADMIN_URLS.PAGES}${page.id}/` : ADMIN_URLS.PAGES}
         className="c-page-explorer__header__title"
         onClick={onClick}
@@ -81,7 +81,7 @@ const PageExplorerHeader: React.FunctionComponent<PageExplorerHeaderProps> = ({
           />
           <span>{page.admin_display_title || gettext('Pages')}</span>
         </div>
-      </Button>
+      </Link>
       {!isSiteRoot &&
         page.meta.locale &&
         page.translations &&

client/src/components/PageExplorer/PageExplorerItem.tsx

@@ -3,7 +3,7 @@ import React from 'react';
 import { gettext } from '../../utils/gettext';
 import { ADMIN_URLS, LOCALE_NAMES } from '../../config/wagtailConfig';
 import Icon from '../Icon/Icon';
-import Button from '../Button/Button';
+import Link from '../Link/Link';
 import PublicationStatus from '../PublicationStatus/PublicationStatus';
 import { PageState } from './reducers/nodes';
 
@@ -35,7 +35,7 @@ const PageExplorerItem: React.FunctionComponent<PageExplorerItemProps> = ({
 
   return (
     <div className="c-page-explorer__item">
-      <Button
+      <Link
         href={`${ADMIN_URLS.PAGES}${id}/`}
         navigate={navigate}
         className="c-page-explorer__item__link"
@@ -49,8 +49,8 @@ const PageExplorerItem: React.FunctionComponent<PageExplorerItemProps> = ({
             {!isPublished && <PublicationStatus status={meta.status} />}
           </span>
         )}
-      </Button>
-      <Button
+      </Link>
+      <Link
         href={`${ADMIN_URLS.PAGES}${id}/edit/`}
         className="c-page-explorer__item__action c-page-explorer__item__action--small"
         navigate={navigate}
@@ -60,9 +60,9 @@ const PageExplorerItem: React.FunctionComponent<PageExplorerItemProps> = ({
           title={gettext("Edit '%(title)s'").replace('%(title)s', title || '')}
           className="icon--item-action"
         />
-      </Button>
+      </Link>
       {hasChildren ? (
-        <Button
+        <Link
           className="c-page-explorer__item__action"
           onClick={onClick}
           href={`${ADMIN_URLS.PAGES}${id}/`}
@@ -76,7 +76,7 @@ const PageExplorerItem: React.FunctionComponent<PageExplorerItemProps> = ({
             )}
             className="icon--item-action"
           />
-        </Button>
+        </Link>
       ) : null}
     </div>
   );

client/src/components/PageExplorer/__snapshots__/PageExplorerHeader.test.js.snap

@@ -4,7 +4,7 @@ exports[`PageExplorerHeader #depth at root 1`] = `
 <div
   className="c-page-explorer__header"
 >
-  <Button
+  <Link
     className="c-page-explorer__header__title"
     href="/admin/pages/undefined/"
     onClick={[MockFunction]}
@@ -20,7 +20,7 @@ exports[`PageExplorerHeader #depth at root 1`] = `
         Pages
       </span>
     </div>
-  </Button>
+  </Link>
 </div>
 `;
 
@@ -28,7 +28,7 @@ exports[`PageExplorerHeader #page 1`] = `
 <div
   className="c-page-explorer__header"
 >
-  <Button
+  <Link
     className="c-page-explorer__header__title"
     href="/admin/pages/a/"
     onClick={[MockFunction]}
@@ -44,7 +44,7 @@ exports[`PageExplorerHeader #page 1`] = `
         test
       </span>
     </div>
-  </Button>
+  </Link>
 </div>
 `;
 
@@ -52,7 +52,7 @@ exports[`PageExplorerHeader basic 1`] = `
 <div
   className="c-page-explorer__header"
 >
-  <Button
+  <Link
     className="c-page-explorer__header__title"
     href="/admin/pages/undefined/"
     onClick={[MockFunction]}
@@ -68,6 +68,6 @@ exports[`PageExplorerHeader basic 1`] = `
         Pages
       </span>
     </div>
-  </Button>
+  </Link>
 </div>
 `;

client/src/components/PageExplorer/__snapshots__/PageExplorerItem.test.js.snap

@@ -4,7 +4,7 @@ exports[`PageExplorerItem children 1`] = `
 <div
   className="c-page-explorer__item"
 >
-  <Button
+  <Link
     className="c-page-explorer__item__link"
     href="/admin/pages/5/"
   >
@@ -17,8 +17,8 @@ exports[`PageExplorerItem children 1`] = `
     >
       test
     </h3>
-  </Button>
-  <Button
+  </Link>
+  <Link
     className="c-page-explorer__item__action c-page-explorer__item__action--small"
     href="/admin/pages/5/edit/"
   >
@@ -27,8 +27,8 @@ exports[`PageExplorerItem children 1`] = `
       name="edit"
       title="Edit 'test'"
     />
-  </Button>
-  <Button
+  </Link>
+  <Link
     className="c-page-explorer__item__action"
     href="/admin/pages/5/"
     onClick={[Function]}
@@ -38,7 +38,7 @@ exports[`PageExplorerItem children 1`] = `
       name="arrow-right"
       title="View child pages of 'test'"
     />
-  </Button>
+  </Link>
 </div>
 `;
 
@@ -46,7 +46,7 @@ exports[`PageExplorerItem renders 1`] = `
 <div
   className="c-page-explorer__item"
 >
-  <Button
+  <Link
     className="c-page-explorer__item__link"
     href="/admin/pages/5/"
   >
@@ -55,8 +55,8 @@ exports[`PageExplorerItem renders 1`] = `
     >
       test
     </h3>
-  </Button>
-  <Button
+  </Link>
+  <Link
     className="c-page-explorer__item__action c-page-explorer__item__action--small"
     href="/admin/pages/5/edit/"
   >
@@ -65,7 +65,7 @@ exports[`PageExplorerItem renders 1`] = `
       name="edit"
       title="Edit 'test'"
     />
-  </Button>
+  </Link>
 </div>
 `;
 
@@ -73,7 +73,7 @@ exports[`PageExplorerItem should show a publication status if not live 1`] = `
 <div
   className="c-page-explorer__item"
 >
-  <Button
+  <Link
     className="c-page-explorer__item__link"
     href="/admin/pages/5/"
   >
@@ -99,8 +99,8 @@ exports[`PageExplorerItem should show a publication status if not live 1`] = `
         }
       />
     </span>
-  </Button>
-  <Button
+  </Link>
+  <Link
     className="c-page-explorer__item__action c-page-explorer__item__action--small"
     href="/admin/pages/5/edit/"
   >
@@ -109,8 +109,8 @@ exports[`PageExplorerItem should show a publication status if not live 1`] = `
       name="edit"
       title="Edit 'test'"
     />
-  </Button>
-  <Button
+  </Link>
+  <Link
     className="c-page-explorer__item__action"
     href="/admin/pages/5/"
     onClick={[Function]}
@@ -120,7 +120,7 @@ exports[`PageExplorerItem should show a publication status if not live 1`] = `
       name="arrow-right"
       title="View child pages of 'test'"
     />
-  </Button>
+  </Link>
 </div>
 `;
 
@@ -128,7 +128,7 @@ exports[`PageExplorerItem should show a publication status with unpublished chan
 <div
   className="c-page-explorer__item"
 >
-  <Button
+  <Link
     className="c-page-explorer__item__link"
     href="/admin/pages/5/"
   >
@@ -154,8 +154,8 @@ exports[`PageExplorerItem should show a publication status with unpublished chan
         }
       />
     </span>
-  </Button>
-  <Button
+  </Link>
+  <Link
     className="c-page-explorer__item__action c-page-explorer__item__action--small"
     href="/admin/pages/5/edit/"
   >
@@ -164,8 +164,8 @@ exports[`PageExplorerItem should show a publication status with unpublished chan
       name="edit"
       title="Edit 'test'"
     />
-  </Button>
-  <Button
+  </Link>
+  <Link
     className="c-page-explorer__item__action"
     href="/admin/pages/5/"
     onClick={[Function]}
@@ -175,6 +175,6 @@ exports[`PageExplorerItem should show a publication status with unpublished chan
       name="arrow-right"
       title="View child pages of 'test'"
     />
-  </Button>
+  </Link>
 </div>
 `;

client/src/index.test.js

@@ -1,5 +1,5 @@
 import {
-  Button,
+  Link,
   Icon,
   PublicationStatus,
   LoadingSpinner,
@@ -8,8 +8,8 @@ import {
 } from './index';
 
 describe('wagtail package API', () => {
-  it('has Button', () => {
-    expect(Button).toBeDefined();
+  it('has Link', () => {
+    expect(Link).toBeDefined();
   });
 
   it('has Icon', () => {

client/src/index.ts

@@ -3,7 +3,7 @@
  * Re-exports components and other modules via a cleaner API.
  */
 
-export { default as Button } from './components/Button/Button';
+export { default as Link } from './components/Link/Link';
 export { default as Icon } from './components/Icon/Icon';
 export { default as LoadingSpinner } from './components/LoadingSpinner/LoadingSpinner';
 export { default as Portal } from './components/Portal/Portal';

docs/releases/6.1.md

@@ -35,6 +35,7 @@ depth: 1
  * Remove duplicate 'path' in default_exclude_fields_in_copy (Ramchandra Shahi Thakuri)
  * Update unit tests to always use the faster, built in `html.parser` & remove `html5lib` dependency (Jake Howard)
  * Adjust Eslint rules for TypeScript files (Karthik Ayangar)
+ * Rename the React `Button` that only renders links (a element) to `Link` and remove unused prop & behavior that was non-compliant for aria role usage (Advik Kabra)
 
 
 ## Upgrade considerations