-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add widget API #1503
Add widget API #1503
Conversation
bc86de2
to
07b912b
Compare
e1d4e3c
to
cb52d94
Compare
.github/workflows/publish-lib.yml
Outdated
{ | ||
echo '{ | ||
"name": "@bluerobotics/cockpit-api", | ||
"version": "'${{ github.ref_name }}'", |
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.
I think we should not use the cockpit version here. thoughts?
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.
We shouldn't. Cockpit has new releases every week, while the API will probably be updated every few months.
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.
I have put some comments here and there, but in general I really like the solution and loved that you added support for Ardupilot variables in the data-lake!
cb52d94
to
13312b8
Compare
13312b8
to
4d6333e
Compare
This is presumably out of scope for this PR, but would this Cockpit API be the basis for supporting widgets registering their own Actions (per #609) in future? :-) |
4d6333e
to
d714e58
Compare
Yes, the idea is to put their metadata in the same json. |
d714e58
to
b86d1cb
Compare
And to allow doing it programatically as we extend the API |
505a55a
to
6c17b99
Compare
src/libs/actions/data-lake.ts
Outdated
if (typeof data !== 'string' && typeof data !== 'number') return | ||
|
||
if (dataLakeVariableInfo[id] === undefined) { | ||
createDataLakeVariable(new DataLakeVariable(id, id, typeof data === 'string' ? 'string' : 'number')) |
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.
Should we keep this? I feel like it creates an incentive for people not to explicitly register their variables.
With the idea of also adding unit information to the variables, this will become more and more of a problem.
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.
I can make it happen only in ardupilot.ts for the mavlink data. I assume we don't want to register all mavlink data manually
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.
Agree, but in this case, to enforce the interface and so we can improve it in the future without having to change a lot of places in the code, I think it would be better to put this "type inferring" in the variable registration and register explicitly in the ardupilot file, even if without the type. It would just be this if clause, but in the ardupilot file instead of the data-lake itself. What do you think?
6c17b99
to
06fe122
Compare
No description provided.