-
Notifications
You must be signed in to change notification settings - Fork 64
feat: Update ChartPanel to use dgrd2 for data display #4511
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: Update ChartPanel to use dgrd2 for data display #4511
Conversation
Removes the need for the dgrid-shim workaround fixes hpcc-systems#4510 Signed-off-by: Gordon Smith <GordonJSmith@gmail.com>
jeclrsg
left a comment
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.
@GordonSmith looked fine to me. just one question about that "extraneous" property in one of the packages in package-lock?
| }, | ||
| "node_modules/@clack/prompts/node_modules/is-unicode-supported": { | ||
| "version": "1.3.0", | ||
| "extraneous": true, |
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.
not familiar with this property, does that mean the package isn't used?
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.
Yes I think so - safe to ignore.
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.
Pull request overview
This PR migrates the ChartPanel component from using the legacy @hpcc-js/dgrid package to the modern @hpcc-js/dgrid2 package, eliminating the need for the dgrid-shim workaround. The change addresses issue #4510 by updating the data table implementation to use a React-based table component instead of the older dojo-based dgrid.
Changes:
- Updated ChartPanel to import Table from
@hpcc-js/dgrid2instead of@hpcc-js/dgrid - Removed dgrid-shim script loading from HTML demo files
- Updated test files to remove dgrid-shim initialization code
- Updated package dependencies to use dgrid2
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/layout/src/ChartPanel.ts | Updated Table import to use dgrid2; added "dgrid2_Table" to legend toggle hide list; removed unused XYAxis import |
| packages/layout/package.json | Replaced @hpcc-js/dgrid dependency with @hpcc-js/dgrid2 version 3.5.3 |
| packages/layout/tests/layout.browser.spec.ts | Removed dgrid-shim loader and associated test; corrected test name from "dgridMod" to "layoutMod" |
| packages/layout/index.html | Removed dgrid-shim CDN script tag |
| packages/layout/index-preview.html | Removed dgrid-shim CDN script tag and dgrid import mapping |
| packages/layout/README.md | Updated comment to remove outdated note about dgrid module dependency limitation |
| package-lock.json | Metadata update for bundled dependency (unrelated to PR changes) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Removes the need for the dgrid-shim workaround
fixes #4510
Checklist:
Testing: