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 input widgets to configurable mini widgets check #1665

Conversation

ArturoManzoli
Copy link
Contributor

Modified the logic that checks if the mini widgets is configurable

Closes #1662

Copy link
Member

@rafaellehmkuhl rafaellehmkuhl left a comment

Choose a reason for hiding this comment

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

Why not to use the same logic that we have for the other two and extend the isMiniWidgetConfigurable object with the types for the input widgets?

@ArturoManzoli
Copy link
Contributor Author

ArturoManzoli commented Feb 5, 2025

Why not to use the same logic that we have for the other two and extend the isMiniWidgetConfigurable object with the types for the input widgets?

Because all input widgets have to be configurable, so creating the type describing that might be unnecessary. What do you think?

@rafaellehmkuhl
Copy link
Member

rafaellehmkuhl commented Feb 5, 2025

Why not to use the same logic that we have for the other two and extend the isMiniWidgetConfigurable object with the types for the input widgets?

Because all input widgets have to be configurable, so creating the type describing that might be unnecessary. What do you think?

I think it would be better to have a single check, as it is today, using the same mecanhism, but if you prefer to use it the way it is, it would be better to create a function (e.g. isWidgetConfigurable(component, type)' so it doesn't grow the number of lines as it's happening now, which degrades code readability.

@ArturoManzoli
Copy link
Contributor Author

I think it would be better to have a single check, as it is today, using the same mecanhism, but if you prefer to use it the way it is, it would be better to create a function (e.g. isWidgetConfigurable(component, type)' so it doesn't grow the number of lines as it's happening now, which degrades code readability.

Agreed, creating the function 👍

@ArturoManzoli ArturoManzoli force-pushed the 1662-input-Widgets-display-having-no-configuration-option branch from 1b648ed to 6c51ab2 Compare February 5, 2025 20:41
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.

Seems to fix the problem - nice! :-)

@ArturoManzoli ArturoManzoli merged commit aa300f3 into bluerobotics:master Feb 6, 2025
11 checks passed
@ArturoManzoli ArturoManzoli deleted the 1662-input-Widgets-display-having-no-configuration-option branch February 6, 2025 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Input Widgets display as having no configuration option
3 participants