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: add TableView codemods #7117

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

feat: add TableView codemods #7117

wants to merge 5 commits into from

Conversation

reidbarber
Copy link
Member

@reidbarber reidbarber commented Sep 27, 2024

  • Updates key to be id
  • Copies columns prop from TableHeader to to Row
  • Updates Row render function to pass column object, not just key

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

@rspbot
Copy link

rspbot commented Sep 27, 2024

@reidbarber reidbarber changed the title feat: add Table codemods feat: add TableView codemods Sep 30, 2024
@rspbot
Copy link

rspbot commented Sep 30, 2024

@rspbot
Copy link

rspbot commented Sep 30, 2024

@rspbot
Copy link

rspbot commented Sep 30, 2024

@devongovett
Copy link
Member

gonna wait until after release for this one so we can test in more apps

@rspbot
Copy link

rspbot commented Oct 8, 2024

<TableHeader>
<Column key="test">Test</Column>
<Column title="Blah">
<Column title="Group 1">
Copy link
Member

Choose a reason for hiding this comment

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

I think RAC/S2 doesn't support nested columns yet.

// Ensure we're not replacing the parameter declaration
innerPath.node !== params[0]
) {
// Replace with 'column.id'
Copy link
Member

Choose a reason for hiding this comment

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

I guess this is sort of an assumption that columns have an id property? They may not, it could be key or something computed. Probably won't be able to be 100% accurate here. I guess if we wanted to get fancy we could try to look at the id (previously key) prop of the Column component? e.g. <Column key={column.name}> and then use column.name here too.

/**
* Updates the key prop to id. Keeps the key prop if it's used in an array.map function.
*/
function updateKeyToId(
Copy link
Member

Choose a reason for hiding this comment

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

Did we not already have something like this for other collection components like Item?

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

Successfully merging this pull request may close these issues.

3 participants