-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
fix: nivo charts version upgrade #6080
base: preview
Are you sure you want to change the base?
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Poem
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
web/core/constants/graph.ts (1)
6-8
: LGTM! Theme property restructuring looks correct.The change from flat
textColor
to nestedtext.color
maintains the same styling while adapting to the new API structure. The value"rgb(var(--color-text-200))"
is preserved, ensuring visual consistency.Consider adding a comment documenting that this theme structure is specific to @nivo v0.88.0+ to help future maintainers understand the version dependency.
web/package.json (1)
24-30
: Consider pinning exact versions with lock fileWhile using caret versions (^0.88.0) provides flexibility for patches and minor updates, it's recommended to:
- Include package-lock.json or yarn.lock in the PR to review exact resolved versions
- Consider pinning exact versions (0.88.0) if the charts are critical to the application's functionality
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (2)
web/core/constants/graph.ts
(1 hunks)web/package.json
(1 hunks)
🔇 Additional comments (2)
web/core/constants/graph.ts (1)
6-8
: Verify theme structure compatibility with @nivo/core v0.88.0
The restructuring of textColor
to text.color
appears to be related to the version upgrade from 0.80.0 to 0.88.0.
Let's verify if there are any other required theme changes:
✅ Verification successful
Theme structure change is correct and consistent across the codebase
The change from textColor
to text.color
is correctly implemented as part of the nivo charts upgrade to v0.88.0. The verification shows:
- The
CHARTS_THEME
constant is used consistently across all graph components (bar, pie, line, calendar, scatter plot) - Each component properly spreads the theme object with potential overrides:
theme={{ ...CHARTS_THEME, ...(theme ?? {}) }}
- The Theme type is correctly imported from '@nivo/core' in all relevant files
- The upgrade to v0.88.0 is consistent across all nivo packages in package.json
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other potential theme properties that might need updating
rg -l "Theme.*from.*@nivo" | xargs rg "textColor"
# Check for any other direct nivo theme usage
ast-grep --pattern 'Theme.*=.*{
$$$
}'
Length of output: 92
Script:
#!/bin/bash
# Search for theme-related configurations in the codebase
rg -A 5 "CHARTS_THEME|chartTheme"
# Search for any nivo-related imports and configurations
rg "@nivo"
# Search for any theme-related type imports from nivo
rg "Theme.*from.*@nivo"
Length of output: 10578
web/package.json (1)
24-30
: Verify breaking changes in nivo upgrade (0.80.0 → 0.88.0)
The significant version jump from 0.80.0 to 0.88.0 across all nivo packages requires careful verification:
- Since these are 0.x.x versions, breaking changes can occur in minor versions
- The AI summary indicates theme-related changes were needed (textColor restructuring)
Let's verify the changelog and migration guides:
✅ Verification successful
Based on the changelog analysis from v0.80.0 to v0.88.0, I can now generate the final response:
Nivo charts upgrade appears safe to proceed
The version upgrade from 0.80.0 to 0.88.0 primarily includes feature additions and bug fixes, with no breaking changes noted in the changelogs. Key changes include:
- Removal of prop-types in favor of TypeScript
- Removal of recompose from the project
- Various feature additions like bar totals, tooltip improvements
- Multiple bug fixes around types and rendering
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes in nivo packages
# Fetch the changelog or releases for the core package
gh api repos/plouc/nivo/releases --jq 'map(select(.tag_name | startswith("v0.8"))) | map({version: .tag_name, body: .body})'
Length of output: 18024
Summary by CodeRabbit
New Features
Chores