Skip to content
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 Data Lake submenu #1594

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rafaellehmkuhl
Copy link
Member

@rafaellehmkuhl rafaellehmkuhl commented Jan 20, 2025

Allows for monitoring of data-lake variables.
The component is completely reactive. If new variables appear, they will be there.
I've added a fuzzy-search input so people can easily find the variables they need.

PR #1595 moves that and the MAVLink submenus to a new "Tools" category, so the current Settings menu will not take more vertical space than it should.

Kapture.2025-01-20.at.15.55.42.mp4

Fix #1451

Copy link
Contributor

@ES-Alexander ES-Alexander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to have this functionality! 😃

It would be useful to provide

  1. a copy-icon button that allows copying the ID of each variable (maybe even with the curly braces), so they can be easily pasted into places that use variables
    • we may also want to show the IDs, but I don't think that's necessary if there's a direct copy button
  2. an edit-icon button that allows changing the value of a variable
    • I'm not sure if there's some way to disable this for automatically generated / fast changing ones, but that's not critical - it can be the user's problem if they try to set a value for something that's getting changed from elsewhere
  3. a + button in the corner, to allow creating new variables
    • Is the data lake persistent? If not, we should include a note/description mentioning that data lake variables are only defined within in a Cockpit session, and do not persist between refreshes

@ES-Alexander ES-Alexander added the docs-needed Change needs to be documented label Jan 21, 2025
@rafaellehmkuhl
Copy link
Member Author

Great to have this functionality! 😃

It would be useful to provide

The first one I totally agree with. Will include.

About the second and third, I actually had them in this branch but decided to remove, because the way that the data-lake works, there's no use case for it despite testing the functionality, as only hardcoded values would be possible and they wouldn't be persistent between sessions and one does not need a data-lake variable for that.

I was thinking we should include them in the future when we add something that can populate data-lake variables, otherwise we will confuse the user.

Copy link
Contributor

@ES-Alexander ES-Alexander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to have the copy buttons, but there's no success feedback so it's not clear whether the button has been successfully pressed.

  • Would it be difficult to make the icon switch to a (green?) tick mark when the button is pressed, before fading/switching back to the copy icon a few seconds later?
    • For reference, GitHub does this when copying the source branch of a PR :-)

Also, it's really disconcerting to have the whole table jiggling constantly (and makes it hard to click the copy icons)

  • can we make the "Current Value" column have a fixed width, and
  • possibly make the float values have a fixed precision?

I don't imagine the values need to be shown at maximum precision here, but if we expect people will actually be using the values from this window then

  • we should add a copy button for the values as well, which can copy the actual value instead of the rounded/clipped display 🤷 🙂

Copy link
Contributor

@ES-Alexander ES-Alexander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • At normal (100%) zoom on the browser, the contents are wider than the panel:
    Screenshot 2025-01-24 at 10 02 22 pm
  • The numbers are still jumpy - can we make them use a mono-spaced font / designate them as <code> or something?

@rafaellehmkuhl
Copy link
Member Author

rafaellehmkuhl commented Jan 24, 2025

  • At normal (100%) zoom on the browser, the contents are wider than the panel:
  • The numbers are still jumpy - can we make them use a mono-spaced font / designate them as <code> or something?

Could you check if you're running the latest version of this branch? I tried simulating different screen sizes and none of them lead to this table sizing.

I also pushed a new version with monospace font and the current values aligned to the left, so numbers with variable precision don't keep jumping.

I don't want to make assumptions about precision/floating-point yet because that can lead to edge-cases on values that should have an awkward number of digits.

Kapture.2025-01-24.at.11.25.34.mp4

Copy link
Contributor

@ES-Alexander ES-Alexander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Re-downloaded and refreshed without cache, and the issue was still there:

    Screen.Recording.2025-01-26.at.11.47.48.pm.mov

    Is the container/scaling of the width based off screen size somehow, rather than window size? It looks fine on my monitor (even with a narrow window), it just gets clipped on my laptop screen.

  • Copying doesn't seem to actually work (and as mentioned previously has no feedback)

We can maybe meet before this week's Cockpit meeting if you want to check anything on my machine :-)

@rafaellehmkuhl
Copy link
Member Author

rafaellehmkuhl commented Jan 27, 2025

  • Re-downloaded and refreshed without cache, and the issue was still there:
    Is the container/scaling of the width based off screen size somehow, rather than window size? It looks fine on my monitor (even with a narrow window), it just gets clipped on my laptop screen.
  • Copying doesn't seem to actually work (and as mentioned previously has no feedback)

About the menu width, there was indeed a problem. I think it's now fixed. Please take a look.
About the copying, I've tested and it's working for me on both browser and electron (both on localhost, running as dev). Are you testing it in a different way?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-needed Change needs to be documented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow directly listing data-lake variables
2 participants