Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: make TableFooter more composeable #634

Merged
merged 2 commits into from Feb 18, 2021
Merged

feat: make TableFooter more composeable #634

merged 2 commits into from Feb 18, 2021

Conversation

raq929
Copy link
Contributor

@raq929 raq929 commented Feb 16, 2021

Users can pass whatever components they want to be displayed in the footer
Defaults to the current RowStatus component on the left and TablePagination on the right

@raq929 raq929 requested a review from a team February 16, 2021 18:45
@coveralls
Copy link

coveralls commented Feb 16, 2021

Coverage Status

Coverage increased (+0.01%) to 88.906% when pulling 933a113 on lbirch/table-footer into 2c5d169 on master.

Comment on lines 93 to 96

.pgn__data-table-footer-row-status {
.pgn__data-table-footer-left {
align-self: $data-table-footer-position;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could do align-items: center to align all child to center by default, removing the need to name or style .pgn__data-table-footer-left

Comment on lines 10 to 13
<LeftComponent
className="pgn__data-table-footer-left"
/>
<TablePagination />
<RightComponent />
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should prefer children here something like:

const TableFooter = ({ children }) => (
  <div className="...png__data-table-footer">
    {children}
  </div>
);

TableFooter.defaultProps = {
  children: (
    <>
      <TablePagination />
      <RowStatus />
    </>
  )
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK so.
Pros: Doing this makes it infinitely flexible. That's nice. It's also in line with how the TableData component is written.
Cons: I think part of having a design system is that you're pushing people to follow good design patterns, and I can Imagine people doing all sorts of things. Also, they can just write their own custom footer at that point?

Copy link
Contributor

Choose a reason for hiding this comment

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

My take:

Make it easy to do the right thing. Make it possible to do something with didn't think of, and trust our team to use this power judiciously. If our components do the right things by default, escape hatches should be infinitely flexible via composability or render props. We should be more concerned with building unnecessarily limiting constraints in our components than with abuse of the flexibility we provide.

  • By default or without configuration components should do the common, desirable thing
  • Desirable variants of a pattern should be easy to create without heavy customization (not related to this particular scenario)
  • Escape hatches should provide as much flexibility as possible.

Copy link
Contributor

@abutterworth abutterworth left a comment

Choose a reason for hiding this comment

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

I think we should avoid LeftComponent RightComponent style props in cases where it's possible, but if this is urgently needed I don't see a reason we can't deliver this value before thinking through another approach.

Users can pass whatever components they want to be displayed in the footer
Defaults to the current RowStatus component on the left and TablePagination on the right
Copy link
Contributor

@abutterworth abutterworth left a comment

Choose a reason for hiding this comment

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

One thing, but lgtm

@raq929 raq929 merged commit e018bc2 into master Feb 18, 2021
@raq929 raq929 deleted the lbirch/table-footer branch February 18, 2021 14:38
@edx-semantic-release
Copy link
Contributor

🎉 This PR is included in version 13.12.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants