Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

AR-1672, AR-1909 Add tr and column-level th/td customization to Table #247

Merged
merged 5 commits into from
Oct 23, 2020

Conversation

justinanastos
Copy link
Contributor

@justinanastos justinanastos commented Oct 21, 2020

Add config for tr, td, and th in Table. Review commit-by-commit with whitespace turned off. I replaced the css= prop with the jsx pragma in Table with the ClassNames component to explicitly allow me to use cx to merge class names.

Resolves issues https://apollographql.atlassian.net/browse/AR-1672 and https://apollographql.atlassian.net/browse/AR-1909

Release Notes

  • Allow trs in the Table component to be customized. You can pass a single trAs prop that accepts keyof JSX.IntrinsicElements, like "tr", or a React.ReactElement that will be cloned with React.cloneElement.

    This configuration accepts a single value that can be applied to both the thead > tr element and to tbody > tr elements. You can also configure them separately with trAs={{ head: ..., body: ... }}, of which both keys are optional (you can exclude either of them and the default of "tr" will be used).

  • Allow each column's td and th to be customized

See the tests and storybook story docs for usage examples.

📦 Published PR as canary version: 7.18.1-canary.247.5724.0

✨ Test out this PR locally via:

npm install @apollo/space-kit@7.18.1-canary.247.5724.0
# or 
yarn add @apollo/space-kit@7.18.1-canary.247.5724.0

@justinanastos justinanastos added the minor Increment the minor version when merged label Oct 21, 2020
@justinanastos justinanastos changed the title AR-1909 Add tr customization to Table AR-1909 Add tr and column-level th/td customization to Table Oct 21, 2020
@justinanastos justinanastos changed the title AR-1909 Add tr and column-level th/td customization to Table AR-1672, AR-1909 Add tr and column-level th/td customization to Table Oct 21, 2020
Copy link
Contributor

@Jephuff Jephuff left a comment

Choose a reason for hiding this comment

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

one little thought, not positive it's better but left the comment there for your perusal :D

I wonder if createElementFromAs might be a nice util to have, but that seems like something that would be best suited for another PR anyways :)

Comment on lines +12 to +18
function createElementFromAs(as: As): React.ReactElement {
return React.isValidElement(as)
? as
: typeof as === "string"
? React.createElement(as)
: assertUnreachable(as);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

NBD
Was trying to think of a way for this to also handle the object as

const bodyTrElement = React.isValidElement(trAs)
    ? trAs
    : typeof trAs === "string"
    ? React.createElement(trAs)
    : createElementFromAs(trAs.body || "tr");

Something like this

Suggested change
function createElementFromAs(as: As): React.ReactElement {
return React.isValidElement(as)
? as
: typeof as === "string"
? React.createElement(as)
: assertUnreachable(as);
}
function createElementFromAs<Key extends string>(
as: As | { [key in Key]?: As } | undefined,
defaultValue: As,
key?: Key
): React.ReactElement {
return React.isValidElement(as)
? as
: typeof as === "string"
? React.createElement(as)
: createElementFromAs((key && as?.[key]) ?? defaultValue, defaultValue);
}

would let use change that to

const bodyTrElement = createElementFromAs(trAs, "tr", "body");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this suggestion! I'm happy the conversation was hot to better handle the object or value instead of should we even have an object or a value 😃 I'm not sure what the right abstraction here would be yet; and I would love to have one because I'd much rather inline the element generation. I'm going to leave this as it is for now and keep thinking about it 😃

Something else I didn't add, but we might want in the future, is another option in As to make the element a function of the row props for the tr, the field props for td, and the column config (?) for the th. Def a use case we can figure out when we need it!

@justinanastos justinanastos merged commit ccccfe4 into main Oct 23, 2020
@justinanastos justinanastos deleted the justin/table-as branch October 23, 2020 15:37
@apollo-bot2
Copy link
Collaborator

🚀 PR was released in v7.19.0 🚀

@apollo-bot2 apollo-bot2 added the released This issue/pull request has been released. label Oct 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
minor Increment the minor version when merged released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants