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

Allow Class Specification on TableColumn #637

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Ramy-Huffman-LPL
Copy link

@Ramy-Huffman-LPL Ramy-Huffman-LPL commented Mar 12, 2024

This is in reference to an enhancement to add to a Table and pass a custom classname.

Release Notes

Author Checklist

  • Add unit test(s)
  • Update version in package.json (see the versioning guidelines)
  • Update documentation (if necessary)
  • Add story to storybook (if necessary)
  • Assign dev reviewer

@chawes13 chawes13 removed their request for review March 27, 2024 17:29
@chawes13
Copy link
Contributor

@Ramy-Huffman-LPL I'm going to pull myself off of the review until this PR is converted from a draft. LMK if you have any questions in the meantime!

@Ramy-Huffman-LPL Ramy-Huffman-LPL marked this pull request as ready for review March 31, 2024 19:42
@chawes13 chawes13 linked an issue Apr 2, 2024 that may be closed by this pull request
@chawes13
Copy link
Contributor

chawes13 commented Apr 2, 2024

@Ramy-Huffman-LPL FYI if you use keywords like "Resolves " in front of your link to the issue, it will not only link it to the Development section of this PR, it will also autoclose it when the PR is merged!

Here's more info: https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue

</SortableTable>
)

const header = screen.getByText('Name').closest('th')
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need closest here? When I printed out the value for screen.getByText('Name'), it was the header itself 🤔

@@ -1,6 +1,6 @@
{
"name": "@launchpadlab/lp-components",
"version": "10.0.1",
"version": "10.1.0",
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 have to qualify this as a breaking change and add some notes to a migration guide.

Any styles that are only applied to a class passed in to a column will be applied to headers as well. In the notes, we should add that if the user does not wish for these styles to apply, then they must include a more specific selector that includes the element, e.g., td.foo {} instead of just .foo {}

@chawes13
Copy link
Contributor

@Ramy-Huffman-LPL will you be able to pick this back up this quarter?

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.

Allow Class Specification on TableColumn
3 participants