upgrade to dndkit v6 #11
Conversation
WalkthroughThe pull request introduces several changes across multiple files, primarily involving the removal of GitHub Actions workflows for Chromatic and size checks. The installation commands have been standardized to use Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
size-limit report 📦
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
.storybook/main.js (2)
7-14: LGTM! Consider updating documentation.The changes to the addons array improve readability and align with the PR objectives. The addition of
@storybook/addon-webpack5-compiler-swccan potentially improve build performance, which is great.Consider updating the project's documentation (e.g., README.md) to mention these new addons and their benefits, especially the integration with Chromatic for visual testing via
@chromatic-com/storybook.
29-29: Clarify the purpose of the empty docs object.An empty
docsobject has been added to the configuration. While this doesn't cause any issues, it also doesn't serve any purpose in its current state.Is this intended as a placeholder for future documentation configuration? If so, consider adding a TODO comment to explain its future use. If not, it might be better to remove it to keep the configuration clean.
package.json (1)
52-52: LGTM! Storybook dependencies successfully updated.The Storybook-related dependencies have been updated to their latest versions, which aligns with the PR objective of upgrading Storybook. The addition of
@storybook/addon-webpack5-compiler-swcsuggests a move to use SWC for faster compilation, which is a good optimization.Consider updating the README.md file if there are any changes in how to run or build Storybook with these new versions.
Also applies to: 54-54, 56-59
stories/SortableList.stories.tsx (1)
88-97: LGTM: New example demonstrates scrolling behavior.The
DraggingScrollingExampleis a valuable addition to the Storybook stories. It effectively demonstrates the SortableList component's ability to handle a larger number of items, which is crucial for testing scrolling behavior.Consider adding a brief comment above the example to explain its purpose, e.g.:
// Demonstrates SortableList behavior with a large number of items to showcase scrolling export const DraggingScrollingExample: React.VFC = () => { // ... (rest of the code) }This would help other developers quickly understand the example's intent.
README.md (3)
Line range hint
41-54: LGTM: Added Tailwind CSS configurationThe addition of Tailwind CSS configuration is a good improvement for users of the library. It ensures that Tailwind styles are properly applied to the components.
Consider adding a brief comment explaining why this configuration is necessary, e.g.:
// Ensure Tailwind CSS styles are applied to the react-sortable-list components
Line range hint
214-241: LGTM: Enhanced ComplexComponentExample with new functionalitiesThe addition of input field editing and a delete button functionality in the ComplexComponentExample is a valuable improvement. It demonstrates more advanced usage of the library.
Consider adding a brief comment explaining the purpose of the new functionalities, e.g.:
// Demonstrate editable fields and item deletion within a sortable listAlso, the delete button's onClick handler currently only shows an alert. Consider updating it to actually remove the item from the list for a more complete example.
Line range hint
262-280: LGTM: Added example for disabling drag and dropThe new example demonstrating the
disabledprop is a valuable addition to the README. It clearly shows users how to disable the drag and drop functionality when needed.Consider adding a brief explanation of when or why a user might want to disable drag and drop. For example:
// Use the `disabled` prop to temporarily or conditionally disable drag and drop functionality // This can be useful for read-only views or when you want to prevent sorting under certain conditions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
example/yarn.lockis excluded by!**/yarn.lock,!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.jsonyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
- .github/workflows/chromatic.yml (1 hunks)
- .storybook/main.js (1 hunks)
- .storybook/preview.js (1 hunks)
- README.md (2 hunks)
- package.json (3 hunks)
- stories/SortableList.stories.tsx (2 hunks)
🧰 Additional context used
🔇 Additional comments (12)
.storybook/preview.js (1)
8-8: Excellent addition of autodocs tag!The addition of
export const tags = ['autodocs'];is a great improvement to the Storybook configuration. This enables automatic documentation generation for all stories by default, which aligns well with the PR objective of upgrading Storybook.Benefits of this change include:
- Improved documentation: Autodocs will automatically generate comprehensive documentation for your components.
- Consistency: Ensures a standardized approach to documentation across all stories.
- Time-saving: Reduces the manual effort required to create and maintain component documentation.
This change will significantly enhance the project's documentation capabilities and improve the developer experience.
.storybook/main.js (2)
24-27: LGTM! Explicit framework configuration.The addition of the
frameworkobject explicitly defining@storybook/react-webpack5is a good practice. This aligns with modern Storybook configuration and can prevent potential issues by clearly specifying the framework being used.
18-22: LGTM! Verify documentation generation.The addition of
reactDocgen: 'react-docgen-typescript'is a good improvement. This will enable automatic prop type documentation for your React components.Please verify that the prop type documentation is being generated correctly for your components. You can do this by checking the Storybook UI for your components. Run the following command to start Storybook and manually verify:
.github/workflows/chromatic.yml (1)
29-29: LGTM: Package manager changeChanging from
yarntonpm installaligns with the PR objective of transitioning from Yarn to NPM for package management. This change is consistent with the project's goals.To ensure consistency across the project, let's verify that we've updated all references to Yarn:
Please ensure that all Yarn references are updated to NPM throughout the project for consistency.
✅ Verification successful
All Yarn references have been successfully updated to NPM, ensuring consistency with the project goals.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to Yarn in the project echo "Checking for Yarn references in package.json scripts:" jq '.scripts | to_entries[] | select(.value | contains("yarn"))' package.json echo -e "\nChecking for Yarn references in README.md:" grep -n "yarn" README.md echo -e "\nChecking for Yarn references in other workflow files:" grep -r "yarn" .github/workflows/ --exclude="chromatic.yml" echo -e "\nChecking for yarn.lock file:" ls yarn.lock 2>/dev/null && echo "yarn.lock file still exists" || echo "No yarn.lock file found (good)"Length of output: 662
package.json (4)
50-50: Please clarify the purpose of adding @chromatic-com/storybook.I noticed the addition of
@chromatic-com/storybookas a dev dependency. While this could potentially enhance our Storybook setup with visual testing capabilities, it wasn't mentioned in the PR objectives.Could you please provide more context on why this dependency was added and how it will be used in the project?
97-97: LGTM! Minor formatting fix.The addition of the closing bracket for the keywords array ensures proper JSON syntax.
99-100: LGTM! @dnd-kit dependencies updated as per PR objective.The updates to
@dnd-kit/core(^6.1.0) and@dnd-kit/sortable(^8.0.0) align with the PR objective of upgrading @dnd-kit/core to version 6 or higher.However, given the major version bump for @dnd-kit/sortable (from 6 to 8), please ensure that:
- All breaking changes have been addressed in the codebase.
- The project's functionality remains intact after these updates.
You can use the following command to check for any usage of potentially changed APIs:
#!/bin/bash # Search for @dnd-kit usage in the codebase grep -R --include="*.{js,jsx,ts,tsx}" "@dnd-kit" src/
18-19: LGTM! Script updates align with project goals.The changes to the scripts section are consistent with the PR objectives:
- Transitioning from Yarn to NPM (lines 21-22).
- Updating Storybook commands to the latest syntax (lines 18-19).
These updates should improve consistency and keep the project up-to-date with the latest Storybook features.
To ensure the Storybook upgrade was successful, let's verify the installed version:
Also applies to: 21-22
✅ Verification successful
Verified Storybook upgrade
The
@storybook/reactdependency has been successfully upgraded to version^8.3.5inpackage.json.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify Storybook version npm list @storybook/react | grep @storybook/reactLength of output: 53
Script:
#!/bin/bash # Verify Storybook version in package.json grep '"@storybook/react"' package.jsonLength of output: 74
stories/SortableList.stories.tsx (1)
83-86: LGTM: Efficient data generation for the new example.The
dataconstant is well-implemented usingArray.from()with a mapping function. This approach efficiently generates a suitable dataset for demonstrating scrolling behavior in the new example.README.md (3)
23-23: LGTM: Updated installation command to use npmThe change from
yarn addtonpm installis consistent with the PR objective of transitioning from Yarn to npm for package management.
276-276: LGTM: Updated development commands to use npmThe development commands have been consistently updated from
yarntonpm, aligning with the PR objective of transitioning to npm for package management.Also applies to: 284-284, 287-289, 295-295
Line range hint
1-315: Overall LGTM: README updates are comprehensive and valuableThe changes to the README are well-executed and align with the PR objectives. Key improvements include:
- Updated installation and development commands to use npm instead of Yarn.
- Added Tailwind CSS configuration instructions.
- Enhanced examples, including the ComplexComponentExample with new functionalities.
- Added a new example for the
disabledprop.These updates provide clear and up-to-date documentation for users of the library.
.github/workflows/chromatic.yml
Outdated
| @@ -24,9 +24,9 @@ jobs: | |||
| - name: Use Node.js | |||
| uses: actions/setup-node@v3 | |||
| with: | |||
| node-version: '16.x' | |||
| node-version: '12.x' | |||
There was a problem hiding this comment.
💡 Codebase verification
Update Node.js version in chromatic.yml to a supported LTS
The node-version in .github/workflows/chromatic.yml is set to '12.x', which is no longer supported and may introduce security vulnerabilities and compatibility issues. To ensure consistency and maintain security across all workflows, update the Node.js version to a supported LTS version, such as '20.x'. Here's the recommended change:
- node-version: '12.x'
+ node-version: '20.x'🔗 Analysis chain
Reconsider downgrading Node.js version
The change from Node.js 16.x to 12.x is concerning. Node.js 12.x reached end-of-life in April 2022, which means it no longer receives security updates or bug fixes. This downgrade could introduce security vulnerabilities and compatibility issues with modern dependencies.
I strongly recommend using a more recent LTS version of Node.js. As of October 2024, consider using Node.js 20.x or the latest LTS version available. Here's a suggested change:
- node-version: '12.x'
+ node-version: '20.x'To ensure we're using an appropriate Node.js version across the project, let's check for any other Node.js version specifications:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Node.js version specifications in package.json, .nvmrc, and other workflow files
echo "Checking package.json for engine specification:"
jq '.engines.node' package.json 2>/dev/null || echo "No Node.js version specified in package.json"
echo -e "\nChecking .nvmrc file:"
cat .nvmrc 2>/dev/null || echo "No .nvmrc file found"
echo -e "\nChecking other workflow files for Node.js version specifications:"
grep -r "node-version:" .github/workflows/ --exclude="chromatic.yml"
Length of output: 536
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
example/package.json (1)
Line range hint
1-24: Consider updating other dependenciesWhile updating the project, it might be beneficial to also update other dependencies to their latest compatible versions, especially dev dependencies like TypeScript and Parcel.
Would you like assistance in generating an updated list of dependencies with their latest compatible versions?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
example/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
- example/package.json (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
example/package.json (3)
12-12:⚠️ Potential issueIncorrect React version specified
The specified React version
"^18.3.1"does not exist. The latest stable version of React is 18.2.0 as of my last update.Please update the React version to a valid, stable release. Here's a suggested fix:
- "react": "^18.3.1", + "react": "^18.2.0",Let's verify the latest stable version of React:
#!/bin/bash # Check the latest stable version of React npm view react version
11-12: Potential conflict with aliased React versionAdding React as a direct dependency might conflict with the existing alias configuration that points to
"../node_modules/react".We should verify if this new direct dependency is necessary and if it might cause any issues with the existing alias. Could you clarify the intention behind adding React as a direct dependency?
#!/bin/bash # Check if React is also listed in the parent package.json if grep -q '"react":' ../package.json; then echo "React is also listed in the parent package.json" grep '"react":' ../package.json else echo "React is not listed in the parent package.json" fi
11-11: Verify the necessity and version of @thaddeusjiang/react-sortable-listThe addition of
"@thaddeusjiang/react-sortable-list": "^0.1.1"seems appropriate for an example project demonstrating this library. However, the version number (0.1.1) suggests it's in an early stage of development.Please confirm if this is the intended version and if there are any stability concerns we should be aware of. Also, could you clarify if this package incorporates the dndkit v6 upgrade mentioned in the PR objectives?
fixed #6 by upgrade to dndkit v6
CleanShot.0006-10-09.at.15.05.15.mp4