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 vehicle discovery #1471

Merged

Conversation

rafaellehmkuhl
Copy link
Member

image

Fix #1314

Turn on security and isolation features and add a preload script.
@ArturoManzoli
Copy link
Contributor

Also, some UI details to fix:

  • In (1), there are repeated expressions "Vehicle Discovery" and "search for vehicles". You could change the text to:
    "This feature allows you to locate and connect to vehicles within your network."

  • In (2), The "Search for vehicles" button should be located on the Vehicke network connection (global address) panel, since its part of that config area, not the user.

  • In (3) The circular loading element should be on the space between the title and the cation "Searching for vehicles in your network", since there is some free space between those two elements. Or it could be where it is if you remove the excessive space between the title and the caption.

  • In (4) '1' and '2' are off-center and '3' could be a filled variant of the button (or another one if you wish) or have a larger margin between the results buttons. This way the user will be able to differentiate those elements intuitively.

(1)
image
(2)
image
(3)
image
(4)
image

Copy link
Contributor

@ArturoManzoli ArturoManzoli left a comment

Choose a reason for hiding this comment

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

For now, while the code is under review, some UI changes are needed.

@rafaellehmkuhl
Copy link
Member Author

Also, some UI details to fix:

  • In (1), there are repeated expressions "Vehicle Discovery" and "search for vehicles". You could change the text to:
    "This feature allows you to locate and connect to vehicles within your network."
  • In (2), The "Search for vehicles" button should be located on the Vehicke network connection (global address) panel, since its part of that config area, not the user.
  • In (3) The circular loading element should be on the space between the title and the cation "Searching for vehicles in your network", since there is some free space between those two elements. Or it could be where it is if you remove the excessive space between the title and the caption.
  • In (4) '1' and '2' are off-center and '3' could be a filled variant of the button (or another one if you wish) or have a larger margin between the results buttons. This way the user will be able to differentiate those elements intuitively.

Nice catches! Will change.

@rafaellehmkuhl
Copy link
Member Author

@ArturoManzoli everything fixed!

@ArturoManzoli
Copy link
Contributor

On electron, when starting a fresh app run, both tutorial and vehicle discovery screens are shown. We should display one at a time.
Maybe vehicle discovery first?

image

Copy link
Contributor

@ArturoManzoli ArturoManzoli left a comment

Choose a reason for hiding this comment

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

Just need to fix the dialog stack!

@rafaellehmkuhl
Copy link
Member Author

Just need to fix the dialog stack!

Taking a further look I could see that actually this is a broader problem. There is already a stack being created on some situations like the one you described because we don't have a central coordination around dialogs and also because the Tutorial dialog is forcing itself to appear even when asked not to because it has an internal logic of auto-appearing (which should be outside it, otherwise the application can't control it). You can see it in the screenshot below (captured on master), that shows the user-select dialog on top of the tutorial dialog:

image

This fix is well beyond the scope of this PR, so I opened #1494 to track that and suggest we go with this as it is.

@ArturoManzoli ArturoManzoli merged commit 75b628c into bluerobotics:master Dec 9, 2024
10 checks passed
@rafaellehmkuhl rafaellehmkuhl deleted the add-vehicle-discovery branch December 9, 2024 15:01
@ES-Alexander ES-Alexander added the docs-needed Change needs to be documented label Dec 10, 2024
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.

Search for BlueOS vehicles in the network and let the user choose between them
3 participants