-
-
Notifications
You must be signed in to change notification settings - Fork 804
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
[PUI] Plugin settings UI #8228
[PUI] Plugin settings UI #8228
Conversation
- Split out into separate files - Use <Accordion /> - Display custom configuration (if available)
✅ Deploy Preview for inventree-web-pui-preview ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
And another example, from the wireviz plugin: |
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.
LGTM in general, there seems to be features removed without a callout in the PR message.
Could we add frontend tests to ensure the integration actually works?
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.
Nice that you have implemented this, this is a great way forward.
However I'm not sure if we really should build lots of similar functionality. Instead, I would prefer if we could build one generic method for integrating custom frontend portions into the ui, so that we don't have to build the same thing (an api endpoint, a plugin function to get the content, a method for rendering a template and providing context from the plugin to the js function, ...) over and over again with different plugin apis (that potentially also confused plugin devs and provides bad dx.
That's why I started to implement the custom ui features as a very generic and abstract interface with custom ts types that can handle various contexts provided by different sources. (With the inventree npm package, this gets even better from a dx perspective)
Maybe for this integration that's a bit overkill to use the custom ui features, but for other ui integrations I would really like to see the generic way improved rather than than implementing the same thing over and over again?
What do you think @matmair ?
@wolflu05 I'm all for refactoring / making this more generic. Maybe we can do that on next feature integration, or as a separate PR? |
@matmair I have slightly tweaked the mechanism for switching between "user settings" / "system settings" / "admin section" - the functionality is still there but visually cleaned up a bit |
Playwright testing has now been added too |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8228 +/- ##
==========================================
- Coverage 84.26% 84.22% -0.05%
==========================================
Files 1160 1162 +2
Lines 52442 52513 +71
Branches 1897 1894 -3
==========================================
+ Hits 44191 44227 +36
- Misses 7790 7821 +31
- Partials 461 465 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…Tree into plugin-settings-ui
Adds functionality to render custom "configuration" UI elements for a plugin, in the new interface.
Expands existing functionality similar to how custom panels are implemented.
The plugin can dynamically render content to this window:
TODO