-
Notifications
You must be signed in to change notification settings - Fork 74
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
plugin API hints #3137
plugin API hints #3137
Conversation
(only hooked up for gaussian smooth for now)
* might want to move to the UI side if we want to control the order and have a copy button
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3137 +/- ##
==========================================
- Coverage 88.83% 88.83% -0.01%
==========================================
Files 112 112
Lines 17430 17436 +6
==========================================
+ Hits 15484 15489 +5
- Misses 1946 1947 +1 ☔ View full report in Codecov by Sentry. |
78357a0
to
226e542
Compare
b8f0ca7
to
4b88362
Compare
Good ideas - I'll give both of those a try next week and we can see how they look. We likely will want to send this to @Jenneh when she's back as well and iterate further on the style as we hear feedback from her and users. For now I was mainly focused on making sure all the different types of input had support and there was some consistent styling to make the API hints standout from everything else. |
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.
The code seems really well done and from my testing everything works as expected, nice job! I think this would benefit from some stress testing during a hack hour and maybe a crash course from you on implementing plugins with these features, but as the work stands it looks good to merge!
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.
Did a second pass on testing and looking through the code, everything checks out!
Description
This pull request implements the infrastructure for in-UI plugin API hints, as well as implementing the hints for a number of (but not all) plugins.
Screen.Recording.2024-08-07.at.9.20.57.AM.mov
The diff is large, but mostly boilerplate. The basic concept is:
api_hints_enabled
traitlet (also available to toggle from the API). This button is added when jdaviz is running in a notebook and the following properties are all passed toj-tray-plugin
:config
andplugin_key
(used to show the line for accessing the plugins fromconfig.plugins[...]
), as well as:api_hints_enabled.sync
.api_hints_enabled
to change the label. In some cases this is done by passingapi_hint
and:api_hints_enabled='api_hints_enabled'
to a re-usable component, and in other cases its done directly inline with:label='api_hints_enabled ? 'plg.whatever = ' : 'Normal Label'
. When manually changing the label/content, theapi-hint
class should also be added (re-usable components do this for you).api-hint
CSS class to style API hints in orange, monospace text to differentiate them from other labels.Change log entry
CHANGES.rst
? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rst
before merge. If no, maintainershould add a
no-changelog-entry-needed
label.Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
trivial
label.