Skip to content

Commit

Permalink
[IndexTable] Allow row click when onClick is passed in on non-selecta…
Browse files Browse the repository at this point in the history
…ble tables (#11763)

<!--
  ☝️How to write a good PR title:
- Prefix it with [ComponentName] (if applicable), for example: [Button]
  - Start with a verb, for example: Add, Delete, Improve, Fix…
  - Give as much context as necessary and as little as possible
  - Open it as a draft if it’s a work in progress
-->

### WHY are these changes introduced?

Resolves #11518 and Relates to
Shopify/credit#1252 which needs to use
clickable IndexTables but without checkboxes.

Here is the
[Figma](https://www.figma.com/file/LtoBWJzLY17yfm9fYxRHrD/Finances-unification?type=design&node-id=4145-35105&mode=design&t=0WNE8MDZa62DwdMj-0)
I'm working off of. Currently these tables exist in Shopify Balance and
Credit already but they're using our own custom css/html alongside
ResourceList and it's
a) hard to manage (the columns don't have the context of the other
columns and don't automatically adjust so we have to hardcode the column
widths which is a pain whenever we need to add a new column
b) doesn't conform to Polaris aesthetics
So we're [trying to
migrate](Shopify/credit#1252) to using
IndexTables to make maintenance on these tables easier, the transition
to the new Figma easier, and the aesthetics of the tables look more like
Polaris.
I have it [mostly implemented
](Shopify/web#120937 the only thing is that
the rows need to be clickable without the checkboxes.

There are no aesthetic changes in this PR for including a screenshot.

### WHAT is this pull request doing?

<!--
  Summary of the changes committed.

Before / after screenshots are appreciated for UI changes. Make sure to
include alt text that describes the screenshot.

  Include a video if your changes include interactive content.

If you include an animated gif showing your change, wrapping it in a
details tag is recommended. Gifs usually autoplay, which can cause
accessibility issues for people reviewing your PR:

  <details>
    <summary>Summary of your gif(s)</summary>
    <img src="..." alt="Description of what the gif shows">
  </details>
-->

This pull requests makes it so that if an `onClick` event is provided to
a `IndexTable.Row` that is in an `IndexTable` that has
`selectable={false}` the `onClick` will still be called. Rows being
selectable vs. clickable should not be mutually exclusive.

### How to 🎩

🖥 [Local development
instructions](https://github.com/Shopify/polaris/blob/main/README.md#install-dependencies-and-build-workspaces)
🗒 [General tophatting
guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md)
📄 [Changelog
guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog)

☝️ TLDR; 
1. `git clone https://github.com/Shopify/polaris.git`
2. `yarn install && cd polaris-react`
3. `dev up && dev server`

I have provided a code snippet that you can put in your playground for
testing. Where in `main` clicking a row would do nothing, with the
changes in this PR clicking on a row will log to the console.

```
    <Page title="Playground">
      <IndexTable
        itemCount={2}
        selectable={false}
        headings={[{title: 'date'}, {title: 'description'}, {title: 'amount'}]}
      >
        <IndexTable.Row onClick={handleOnClick} position={1} id="1">
          <IndexTable.Cell>Today</IndexTable.Cell>
          <IndexTable.Cell>Dollarama</IndexTable.Cell>
          <IndexTable.Cell>$30</IndexTable.Cell>
        </IndexTable.Row>
      </IndexTable>
    </Page>
```

You can also test it on
[this](https://admin.web.banking-credit-f7h4.sydney-rhude.us.spin.dev/store/shop3/credit?referrer=finances-nav&modalView=receiptView)
spin instance (the transaction table shown utilizes non-selectable but
yes clickable)

### 🎩 checklist

I'm not so sure this needs to be added to the changeset, documentation,
or readme, as it is just implementing a fix for something that should
already exist but doesn't? I'm not sure. This isn't a new feature, it's
just a fix for an existing one that rarely gets used I guess?

- [x] Tested a
[snapshot](https://github.com/Shopify/polaris/blob/main/documentation/Releasing.md#-snapshot-releases)
- [ ] Tested on
[mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing)
- [ ] Tested on [multiple
browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers)
- [ ] Tested for
[accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md)
- [ ] Updated the component's `README.md` with documentation changes
- [ ] [Tophatted
documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md)
changes in the style guide

---------

Co-authored-by: Chloe Rice <[email protected]>
  • Loading branch information
sydturn and chloerice authored Mar 20, 2024
1 parent 1c9eb9c commit 18d0b02
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 2 deletions.
5 changes: 5 additions & 0 deletions .changeset/odd-swans-teach.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/polaris': patch
---

Fixed `IndexTable.Row` `onClick` not being called when `selectable` is `false`
Original file line number Diff line number Diff line change
Expand Up @@ -120,13 +120,14 @@ export const Row = memo(function Row({
disabled && styles['TableRow-disabled'],
tone && styles[variationName('tone', tone)],
!selectable &&
!onClick &&
!primaryLinkElement.current &&
styles['TableRow-unclickable'],
);

let handleRowClick;

if ((!disabled && selectable) || primaryLinkElement.current) {
if ((!disabled && selectable) || onClick || primaryLinkElement.current) {
handleRowClick = (event: React.MouseEvent) => {
if (rowType === 'subheader') return;

Expand All @@ -141,7 +142,7 @@ export const Row = memo(function Row({
return;
}

if (primaryLinkElement.current && !selectMode) {
if (primaryLinkElement.current && !selectMode && selectable) {
isNavigating.current = true;
const {ctrlKey, metaKey} = event.nativeEvent;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,27 @@ describe('<Row />', () => {
);
});

it('fires onClick handler when row has an onclick and is clicked despite no primary link child present and the table not being selectable', () => {
const mockOnClick = jest.fn();
const row = mountWithTable(
<Row {...defaultProps} onClick={mockOnClick}>
<th>
<a href="/">Child without data-primary-link</a>
</th>
</Row>,
{
indexTableProps: {
itemCount: 1,
selectable: false,
},
},
);

triggerOnClick(row, 1, defaultEvent);

expect(mockOnClick).toHaveBeenCalled();
});

it('has an undefined tone by default', () => {
const row = mountWithTable(
<Row {...defaultProps}>
Expand Down

0 comments on commit 18d0b02

Please sign in to comment.