Skip to content

Conversation

devonpmack
Copy link
Contributor

@devonpmack devonpmack commented Jan 20, 2020

WHY are these changes introduced?

Fixes #2597

We needed check boxes in our header but it was not supported. (Part of https://github.com/Shopify/store/issues/12522)

WHAT is this pull request doing?

Allow the header prop of DataTable to receive and display React Nodes.

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import React from "react";
import { Card, DataTable, Page, Button } from "@shopify/polaris";

export default function DataTableExample() {
  return (
    <Page title="Sales by product">
      <Card>
        <DataTable
          columnContentTypes={[Array(5).fill("text")]}
          headings={Array(5).fill(<Button />)}
          rows={[]}
        />
      </Card>
    </Page>
  );
}

🎩 checklist

  • Tested on mobile
  • Tested on multiple browsers
  • Tested for accessibility
  • Updated the component's README.md with documentation changes
  • Tophatted documentation changes in the style guide
  • For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

@github-actions
Copy link
Contributor

github-actions bot commented Jan 20, 2020

💦 Potential splash zone of changes introduced to src/**/*.tsx in this pull request:

No significant changes to src/**/*.tsx were detected.


This comment automatically updates as changes are made to this pull request.
Feedback, troubleshooting: open an issue or reach out on Slack in #polaris-tooling.

Copy link
Contributor

@PabloVallejo PabloVallejo left a comment

Choose a reason for hiding this comment

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

🎩 Working correctly! and code looks 👌

image

maybe if we should write a test passing a react node to the headings!

@devonpmack
Copy link
Contributor Author

@PabloVallejo Good call, I just added a test and example to component README.

Copy link
Contributor

@PabloVallejo PabloVallejo left a comment

Choose a reason for hiding this comment

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

Test and README ✅ 💯

@devonpmack devonpmack force-pushed the datatable-headers-allow-jsx branch from fcd77a2 to 3447e1d Compare January 21, 2020 15:54
Copy link
Member

@BPScott BPScott left a comment

Choose a reason for hiding this comment

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

You can simplify the typing to just React.ReactNode[].

Petty but this appears in a few places so it's worth being as simple as possible :)

Fix up that and then this should be good to go

@devonpmack
Copy link
Contributor Author

Thanks! I forgot that string was included. I've updated it to include your suggestion.

@devonpmack devonpmack force-pushed the datatable-headers-allow-jsx branch from bc223f5 to 56ef852 Compare January 22, 2020 15:01
@devonpmack devonpmack requested a review from BPScott January 22, 2020 15:02
@devonpmack devonpmack force-pushed the datatable-headers-allow-jsx branch from 56ef852 to 73a8d3a Compare January 22, 2020 17:09
@dleroux dleroux self-requested a review January 22, 2020 17:10
Copy link
Contributor

@dleroux dleroux left a comment

Choose a reason for hiding this comment

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

:shipit:

@devonpmack devonpmack merged commit 22f35b0 into master Jan 22, 2020
@devonpmack devonpmack deleted the datatable-headers-allow-jsx branch January 22, 2020 17:56
@dleroux dleroux temporarily deployed to production January 27, 2020 13:38 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[DataTable] Allow JSX elements in the header

4 participants