-
Notifications
You must be signed in to change notification settings - Fork 33
feat: DH-19146: ElementPlugin mapping hook #2477
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: DH-19146: ElementPlugin mapping hook #2477
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2477 +/- ##
==========================================
+ Coverage 44.62% 44.64% +0.02%
==========================================
Files 759 760 +1
Lines 42550 42564 +14
Branches 10693 10696 +3
==========================================
+ Hits 18989 19004 +15
+ Misses 23550 23549 -1
Partials 11 11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
Adds support for extracting element-specific plugins by introducing a new hook, utility function, type definitions, and associated tests.
- Introduces
useElementPluginMapping
hook to build a mapping of element names to their React components from the plugin context. - Extends
PluginUtils
andPluginTypes
withElementPlugin
support and thegetElementPluginMapping
helper. - Adds unit tests for the new hook and
getElementPluginMapping
function.
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
packages/plugin/src/useElementPluginMapping.tsx | New hook to retrieve element plugin mapping |
packages/plugin/src/index.ts | Exports the new hook |
packages/plugin/src/UseElementPluginMapping.test.ts | Tests for the new hook |
packages/plugin/src/PluginUtils.tsx | Added getElementPluginMapping and logging |
packages/plugin/src/PluginUtils.test.tsx | Tests for getElementPluginMapping |
packages/plugin/src/PluginTypes.ts | Defined ElementPlugin type and isElementPlugin |
Comments suppressed due to low confidence (2)
packages/plugin/src/UseElementPluginMapping.test.ts:1
- [nitpick] Test file name casing (
UseElementPluginMapping.test.ts
) differs from the hook file (useElementPluginMapping.tsx
). Consider aligning test file name touseElementPluginMapping.test.ts
for consistency.
import { renderHook } from '@testing-library/react-hooks';
packages/plugin/src/PluginUtils.tsx:89
- React is not imported in this file, but
React.ComponentType
is used. Addimport { type ComponentType } from 'react';
orimport React from 'react';
at the top.
): Map<string, React.ComponentType<unknown>> {
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.
A few small things to clean up
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.
Couple other things I noticed after reviewing the deephaven-plugins side.
7be48fb
to
003027c
Compare
Will also need deephaven/deephaven-plugins#1201 for the plugin side, but this implements a hook that pulls the necessary mapping for element plugins