UI: Remove stretchable link from build and buildset table

Making the whole row table row clickable in these pages makes makes it
impossible to mark & copy any text from those rows.  Users have
expressed this is an annoyance worth fixing.

This change removes the stretchable link from the whole table row and
instead allows clicking on the job name or status icon to go to
details about the job.

We have added an optional link prop to title-with-icon; note this is
used in other contexts where it does not make sense to be a link thus
the optional implementation steps.  It is intentional that these links
retain their underline for discoverability.

For the mobile version (which renders the table as a nice card list), we
fall back to the "action column" solution which will be rendered nicely
as a button in the top right corner of each card. For larger screens we
hide this action column.

Note this change is a squash of several other changes to make a
consolidated review; for original details see:

 I788ea37b2ca1899d90160cc52761db0a77f78508
 Ifc54edf9ca348ada8e3bfd07fd135d22f9f08489
 Ifc54edf9ca348ada8e3bfd07fd135d22f9f08489
 Ib4c3a2159e7bad58fcc27eb3ba4b62926ce73099

Co-Authored-By: Ian Wienand <iwienand@redhat.com>
Change-Id: I276ff316b17aee932f2724f86e8f0eb1abe73c60

7 files changed, 143 insertions, 152 deletions

diff --git a/web/src/Misc.jsx b/web/src/Misc.jsx
index ba2d62b02..e209ed042 100644
--- a/web/src/Misc.jsx
+++ b/web/src/Misc.jsx
@@ -92,4 +92,9 @@ function buildExternalTableLink(buildish) {
   return null
 }
 
-export { removeHash, ExternalLink, buildExternalLink, buildExternalTableLink }
+// https://github.com/kitze/conditional-wrap
+// appears to be the first implementation of this pattern
+const ConditionalWrapper = ({ condition, wrapper, children }) =>
+  condition ? wrapper(children) : children
+
+export { removeHash, ExternalLink, buildExternalLink, buildExternalTableLink, ConditionalWrapper }
diff --git a/web/src/containers/build/BuildTable.jsx b/web/src/containers/build/BuildTable.jsx
index 00e6b3eba..c7cb15601 100644
--- a/web/src/containers/build/BuildTable.jsx
+++ b/web/src/containers/build/BuildTable.jsx
@@ -15,7 +15,6 @@
 import React from 'react'
 import PropTypes from 'prop-types'
 import { connect } from 'react-redux'
-import { Link } from 'react-router-dom'
 import {
   Button,
   EmptyState,
@@ -47,8 +46,14 @@ import * as moment from 'moment'
 import { BuildResult, BuildResultWithIcon, IconProperty } from './Misc'
 import { buildExternalTableLink } from '../../Misc'
 
-function BuildTable(props) {
-  const { builds, fetching, onClearFilters, tenant, timezone } = props
+function BuildTable({
+  builds,
+  fetching,
+  onClearFilters,
+  tenant,
+  timezone,
+  history,
+}) {
   const columns = [
     {
       title: <IconProperty icon={<BuildIcon />} value="Job" />,
@@ -87,28 +92,23 @@ function BuildTable(props) {
   ]
 
   function createBuildRow(build) {
-    // This link will be defined on each cell of the current row as this is the
-    // only way to define a valid HTML link on a table row. Although we could
-    // simply define an onClick handler on the whole row and programatically
-    // switch to the buildresult page, this wouldn't provide the same
-    // look-and-feel as a plain HTML link.
-    const buildResultLink = (
-      <Link
-        to={`${tenant.linkPrefix}/build/${build.uuid}`}
-        className="zuul-stretched-link"
-      />
-    )
-    const build_link = buildExternalTableLink(build)
+    const changeOrRefLink = buildExternalTableLink(build)
 
     return {
+      // Pass the build's uuid as row id, so we can use it later on in the
+      // action handler to build the link to the build result page for each row.
+      id: build.uuid,
       cells: [
         {
           // To allow passing anything else than simple string values to a table
           // cell, we must use the title attribute.
           title: (
             <>
-              {buildResultLink}
-              <BuildResultWithIcon result={build.result} colored={build.voting}>
+              <BuildResultWithIcon
+                result={build.result}
+                colored={build.voting}
+                link={`${tenant.linkPrefix}/build/${build.uuid}`}
+              >
                 {build.job_name}
                 {!build.voting && ' (non-voting)'}
               </BuildResultWithIcon>
@@ -116,74 +116,36 @@ function BuildTable(props) {
           ),
         },
         {
-          title: (
-            <>
-              {buildResultLink}
-              <span>{build.project}</span>
-            </>
-          ),
+          title: build.project,
         },
         {
-          title: (
-            <>
-              {buildResultLink}
-              <span>{build.branch ? build.branch : build.ref}</span>
-            </>
-          ),
+          title: build.branch ? build.branch : build.ref,
         },
         {
-          title: (
-            <>
-              {buildResultLink}
-              <span>{build.pipeline}</span>
-            </>
-          ),
+          title: build.pipeline,
         },
         {
-          title: (
-            <>
-              {buildResultLink}
-              {build_link && (
-                <span style={{ zIndex: 1, position: 'relative' }}>
-                  {build_link}
-                </span>
-              )}
-            </>
-          ),
+          title: changeOrRefLink && changeOrRefLink,
         },
         {
-          title: (
-            <>
-              {buildResultLink}
-              <span>
-                {moment
-                  .duration(build.duration, 'seconds')
-                  .format('h [hr] m [min] s [sec]')}
-              </span>
-            </>
-          ),
+          title: moment
+            .duration(build.duration, 'seconds')
+            .format('h [hr] m [min] s [sec]'),
         },
         {
-          title: (
-            <>
-              {buildResultLink}
-              <span>
-                {build.start_time &&
-                  moment
-                    .utc(build.start_time)
-                    .tz(timezone)
-                    .format('YYYY-MM-DD HH:mm:ss')}
-              </span>
-            </>
-          ),
+          title: moment
+            .utc(build.start_time)
+            .tz(timezone)
+            .format('YYYY-MM-DD HH:mm:ss'),
         },
         {
-          title: (
-            <>
-              {buildResultLink}
-              <BuildResult result={build.result} colored={build.voting} />
-            </>
-          ),
+            title: (
+              <BuildResult
+                result={build.result}
+                link={`${tenant.linkPrefix}/build/${build.uuid}`}
+                colored={build.voting}
+              />
+            ),
         },
       ],
     }
@@ -209,6 +171,9 @@ function BuildTable(props) {
   }
 
   let rows = []
+  // For the fetching row we don't need any actions, so we keep them empty by
+  // default.
+  let actions = []
   if (fetching) {
     rows = createFetchingRow()
     // The dataLabel property is used to show the column header in a list-like
@@ -219,6 +184,18 @@ function BuildTable(props) {
     columns[0].dataLabel = ''
   } else {
     rows = builds.map((build) => createBuildRow(build))
+    // This list of actions will be applied to each row in the table. For
+    // row-specific actions we must evaluate the individual row data provided to
+    // the onClick handler.
+    actions = [
+      {
+        title: 'Show build result',
+        onClick: (event, rowId, rowData) =>
+          // The row's id contains the build's uuid, so we can use this to build
+          // the correct link.
+          history.push(`${tenant.linkPrefix}/build/${rowData.id}`),
+      },
+    ]
   }
 
   return (
@@ -228,6 +205,7 @@ function BuildTable(props) {
         variant={TableVariant.compact}
         cells={columns}
         rows={rows}
+        actions={actions}
         className="zuul-build-table"
       >
         <TableHeader />
@@ -261,6 +239,7 @@ BuildTable.propTypes = {
   onClearFilters: PropTypes.func.isRequired,
   tenant: PropTypes.object.isRequired,
   timezone: PropTypes.string.isRequired,
+  history: PropTypes.object.isRequired,
 }
 
 export default connect((state) => ({
diff --git a/web/src/containers/build/BuildsetTable.jsx b/web/src/containers/build/BuildsetTable.jsx
index dcd7efb0c..f0947971d 100644
--- a/web/src/containers/build/BuildsetTable.jsx
+++ b/web/src/containers/build/BuildsetTable.jsx
@@ -15,7 +15,6 @@
 import React from 'react'
 import PropTypes from 'prop-types'
 import { connect } from 'react-redux'
-import { Link } from 'react-router-dom'
 import {
   Button,
   EmptyState,
@@ -43,8 +42,13 @@ import {
 import { BuildResult, BuildResultWithIcon, IconProperty } from './Misc'
 import { buildExternalTableLink } from '../../Misc'
 
-function BuildsetTable(props) {
-  const { buildsets, fetching, onClearFilters, tenant } = props
+function BuildsetTable({
+  buildsets,
+  fetching,
+  onClearFilters,
+  tenant,
+  history,
+}) {
   const columns = [
     {
       title: <IconProperty icon={<CubeIcon />} value="Project" />,
@@ -69,68 +73,41 @@ function BuildsetTable(props) {
   ]
 
   function createBuildsetRow(buildset) {
-    // This link will be defined on each cell of the current row as this is the
-    // only way to define a valid HTML link on a table row. Although we could
-    // simply define an onClick handler on the whole row and programatically
-    // switch to the buildresult page, this wouldn't provide the same
-    // look-and-feel as a plain HTML link.
-    const buildsetResultLink = (
-      <Link
-        to={`${tenant.linkPrefix}/buildset/${buildset.uuid}`}
-        className="zuul-stretched-link"
-      />
-    )
-    const buildset_link = buildExternalTableLink(buildset)
+    const changeOrRefLink = buildExternalTableLink(buildset)
 
     return {
+      // Pass the buildset's uuid as row id, so we can use it later on in the
+      // action handler to build the link to the build result page for each row.
+      id: buildset.uuid,
       cells: [
         {
           // To allow passing anything else than simple string values to a table
           // cell, we must use the title attribute.
           title: (
-            <>
-              {buildsetResultLink}
-              <BuildResultWithIcon result={buildset.result}>
-                {buildset.project}
-              </BuildResultWithIcon>
-            </>
+            <BuildResultWithIcon
+              result={buildset.result}
+              link={`${tenant.linkPrefix}/buildset/${buildset.uuid}`}>
+              {buildset.project}
+            </BuildResultWithIcon>
           ),
         },
         {
-          title: (
-            <>
-              {buildsetResultLink}
-              <span>{buildset.branch ? buildset.branch : buildset.ref}</span>
-            </>
-          ),
+          title: buildset.branch ? buildset.branch : buildset.ref,
         },
         {
-          title: (
-            <>
-              {buildsetResultLink}
-              <span>{buildset.pipeline}</span>
-            </>
-          ),
+          title: buildset.pipeline,
         },
         {
-          title: (
-            <>
-              {buildsetResultLink}
-              {buildset_link && (
-                <span style={{ zIndex: 1, position: 'relative' }}>
-                  {buildset_link}
-                </span>
-              )}
-            </>
-          ),
+          title: changeOrRefLink && changeOrRefLink,
         },
         {
-          title: (
-            <>
-              {buildsetResultLink}
-              <BuildResult result={buildset.result} />
-            </>
-          ),
+            title: (
+              <BuildResult
+                result={buildset.result}
+                link={`${tenant.linkPrefix}/buildset/${buildset.uuid}`}
+              >
+              </BuildResult>
+            ),
         },
       ],
     }
@@ -156,6 +133,9 @@ function BuildsetTable(props) {
   }
 
   let rows = []
+  // For the fetching row we don't need any actions, so we keep them empty by
+  // default.
+  let actions = []
   if (fetching) {
     rows = createFetchingRow()
     // The dataLabel property is used to show the column header in a list-like
@@ -166,6 +146,18 @@ function BuildsetTable(props) {
     columns[0].dataLabel = ''
   } else {
     rows = buildsets.map((buildset) => createBuildsetRow(buildset))
+    // This list of actions will be applied to each row in the table. For
+    // row-specific actions we must evaluate the individual row data provided to
+    // the onClick handler.
+    actions = [
+      {
+        title: 'Show buildset result',
+        onClick: (event, rowId, rowData) =>
+          // The row's id contains the buildset's uuid, so we can use it to
+          // build the correct link.
+          history.push(`${tenant.linkPrefix}/buildset/${rowData.id}`),
+      },
+    ]
   }
 
   return (
@@ -175,6 +167,7 @@ function BuildsetTable(props) {
         variant={TableVariant.compact}
         cells={columns}
         rows={rows}
+        actions={actions}
         className="zuul-build-table"
       >
         <TableHeader />
@@ -207,6 +200,7 @@ BuildsetTable.propTypes = {
   fetching: PropTypes.bool.isRequired,
   onClearFilters: PropTypes.func.isRequired,
   tenant: PropTypes.object.isRequired,
+  history: PropTypes.object.isRequired,
 }
 
 export default connect((state) => ({ tenant: state.tenant }))(BuildsetTable)
diff --git a/web/src/containers/build/Misc.jsx b/web/src/containers/build/Misc.jsx
index f77f7de7a..1d02992c0 100644
--- a/web/src/containers/build/Misc.jsx
+++ b/web/src/containers/build/Misc.jsx
@@ -14,13 +14,17 @@
 
 import * as React from 'react'
 import PropTypes from 'prop-types'
-import { Label } from '@patternfly/react-core'
+import { Link } from 'react-router-dom'
+import {
+    Label,
+} from '@patternfly/react-core'
 import {
   CheckIcon,
   ExclamationIcon,
   QuestionIcon,
   TimesIcon,
 } from '@patternfly/react-icons'
+import { ConditionalWrapper } from '../../Misc'
 
 const RESULT_ICON_CONFIGS = {
   SUCCESS: {
@@ -82,15 +86,25 @@ const DEFAULT_RESULT_ICON_CONFIG = {
 }
 
 function BuildResult(props) {
-  const { result, colored = true } = props
+  const { result, link = undefined, colored = true } = props
   const iconConfig = RESULT_ICON_CONFIGS[result] || DEFAULT_RESULT_ICON_CONFIG
   const color = colored ? iconConfig.color : 'inherit'
 
-  return <span style={{ color: color }}>{result}</span>
+  return (
+    <span style={{ color: color }}>
+      <ConditionalWrapper
+        condition={link}
+        wrapper={children => <Link to={link} style={{ color: color }}>{children}</Link>}
+      >
+        {result}
+      </ConditionalWrapper>
+    </span>
+  )
 }
 
 BuildResult.propTypes = {
   result: PropTypes.string,
+  link: PropTypes.string,
   colored: PropTypes.bool,
 }
 
@@ -117,7 +131,7 @@ BuildResultBadge.propTypes = {
 }
 
 function BuildResultWithIcon(props) {
-  const { result, colored = true, size = 'sm' } = props
+  const { result, link = undefined, colored = true, size = 'sm' } = props
   const iconConfig = RESULT_ICON_CONFIGS[result] || DEFAULT_RESULT_ICON_CONFIG
 
   // Define the verticalAlign based on the size
@@ -132,20 +146,26 @@ function BuildResultWithIcon(props) {
 
   return (
     <span style={{ color: color }}>
-      <Icon
-        size={size}
-        style={{
-          marginRight: 'var(--pf-global--spacer--sm)',
-          verticalAlign: verticalAlign,
-        }}
-      />
-      {props.children}
+      <ConditionalWrapper
+        condition={link}
+        wrapper={children => <Link to={link} style={{ color: color }}>{children}</Link>}
+      >
+        <Icon
+          size={size}
+          style={{
+            marginRight: 'var(--pf-global--spacer--sm)',
+            verticalAlign: verticalAlign,
+          }}
+        />
+        {props.children}
+      </ConditionalWrapper>
     </span>
   )
 }
 
 BuildResultWithIcon.propTypes = {
   result: PropTypes.string,
+  link: PropTypes.string,
   colored: PropTypes.bool,
   size: PropTypes.string,
   children: PropTypes.node,
diff --git a/web/src/index.css b/web/src/index.css
index 83c73b613..2f6602c1c 100644
--- a/web/src/index.css
+++ b/web/src/index.css
@@ -51,10 +51,6 @@ a.refresh {
     font-weight: bold;
 }
 
-.zuul-build-list:hover a {
-    text-decoration: none;
-}
-
 /* Keep the normal font-size for compact tables */
 .zuul-build-table td {
     font-size: var(--pf-global--FontSize--md);
@@ -98,20 +94,13 @@ a.refresh {
     .zuul-build-table tbody tr:hover td:first-child::before {
         background-color: var(--pf-global--active-color--100);
     }
-}
 
-/* Make a link stretch the whole parent element (in this case the whole table
-   cell) */
-.zuul-stretched-link::after {
-    position: absolute;
-    top: 0;
-    right: 0;
-    bottom: 0;
-    left: 0;
-    z-index: 0;
-    pointer-events: auto;
-    content: "";
-    background-color: rgba(0,0,0,0);
+    /* Hide the action column with the build link on larger screen. This is only
+       needed for the mobile version as we can't use the "magnifying-glass icon
+       on hover" effect there. */
+    .zuul-build-table .pf-c-table__action {
+        display: none;
+    }
 }
 
 /*
diff --git a/web/src/pages/Builds.jsx b/web/src/pages/Builds.jsx
index 144f9ab85..bb4e4cf47 100644
--- a/web/src/pages/Builds.jsx
+++ b/web/src/pages/Builds.jsx
@@ -151,6 +151,7 @@ class BuildsPage extends React.Component {
   }
 
   render() {
+    const { history } = this.props
     const { builds, fetching, filters } = this.state
     return (
       <PageSection variant={PageSectionVariants.light}>
@@ -163,6 +164,7 @@ class BuildsPage extends React.Component {
           builds={builds}
           fetching={fetching}
           onClearFilters={this.handleClearFilters}
+          history={history}
         />
       </PageSection>
     )
diff --git a/web/src/pages/Buildsets.jsx b/web/src/pages/Buildsets.jsx
index 602f4d0d8..a9372e8b0 100644
--- a/web/src/pages/Buildsets.jsx
+++ b/web/src/pages/Buildsets.jsx
@@ -139,6 +139,7 @@ class BuildsetsPage extends React.Component {
   }
 
   render() {
+    const { history } = this.props
     const { buildsets, fetching, filters } = this.state
     return (
       <PageSection variant={PageSectionVariants.light}>
@@ -151,6 +152,7 @@ class BuildsetsPage extends React.Component {
           buildsets={buildsets}
           fetching={fetching}
           onClearFilters={this.handleClearFilters}
+          history={history}
         />
       </PageSection>
     )