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

Show in the edit-menu the mini-widgets that are out of the top/bottom bar #1559

Conversation

ArturoManzoli
Copy link
Contributor

@ArturoManzoli ArturoManzoli commented Jan 9, 2025

  • Added a 'double click to configure' feature to mini and regular widgets inside edit mode, so any configurable widget will have a direct way to open its config screen;
  • Added a panel to list and edit mini-widgets inside custom mini-widgets containers;
Screenshare.-.2025-01-09.3_02_09.PM.mp4

Fix #1531

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.

Added a 'double click to configure' feature to mini and regular widgets inside edit mode, so any configurable widget will have a direct way to open its config screen;

This feature works, but is there a reason to require double-clicks, instead of single clicks? Double clicks are awkward on mobile, and the extra click seems unnecessary, so I suggested single clicks in #1540.

Added a panel to list and edit mini-widgets inside custom mini-widgets containers;

The panel appears and lists the mini-widgets, and they can be deleted, but clicking the cog does not open the configuration.

Also, hovering the cards or the mini-widgets does not highlight them. I've confirmed that the mini-widget bar seems to absorb the highlight hover event intended for the mini-widgets it contains in stable as well, so we could potentially treat this as a separate bug, but that doesn't necessarily explain why hovering the listed card fails to work with highlighting at all.

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.

Agree with Eliot that we need to make things consistent. The primary configuration interface is through the cog icon, so we should use it, otherwise we are confusing users.

@ArturoManzoli ArturoManzoli force-pushed the 1531-mini-widgets-outside-header-footer-arent-accessible branch from 597105d to 224c547 Compare January 14, 2025 19:48
@ArturoManzoli
Copy link
Contributor Author

ArturoManzoli commented Jan 14, 2025

On last push:

  • Removed the 'double click to configure' for regular and mini-widgets;
  • Added mini widgets custom bars as source of real mini widgets, so they will be configurable and highlighted on a mouse hover.

@ArturoManzoli ArturoManzoli force-pushed the 1531-mini-widgets-outside-header-footer-arent-accessible branch from 224c547 to 0cff2e1 Compare January 14, 2025 19:53
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 work as expected - nice work! 😃

The remaining things I'd want changed are already mentioned in other Issues, and are out of scope for the purpose of this PR.

@rafaellehmkuhl rafaellehmkuhl changed the title 1531 mini widgets outside header footer aren't accessible Add a panel to list and edit mini-widgets inside custom mini-widgets containers Jan 15, 2025
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.

It seems like we are adding the panel for the MiniWidgetsBar but not for the CustomWidgetBase.

image

@ES-Alexander
Copy link
Contributor

ES-Alexander commented Jan 15, 2025

It seems like we are adding the panel for the MiniWidgetsBar but not for the CustomWidgetBase.

@rafaellehmkuhl Is that expected to work properly for general mini-widgets yet? I remember us having a discussion about Input Widgets being considered as a subset of mini-widgets, but was under the impression they're currently (still) independent, and the CustomWidgetBase is only intended for Input Widgets for now, and Mini Widget bars are only intended for standard mini-widgets.

I do agree it should at least get treated as a container, and list its elements - just noting that your screenshot example used a normal mini-widget in there.

@rafaellehmkuhl
Copy link
Member

It seems like we are adding the panel for the MiniWidgetsBar but not for the CustomWidgetBase.

@rafaellehmkuhl Is that expected to work properly for general mini-widgets yet? I remember us having a discussion about Input Widgets being considered as a subset of mini-widgets, but was under the impression they're currently (still) independent, and the CustomWidgetBase is only intended for Input Widgets for now, and Mini Widget bars are only intended for standard mini-widgets.

I do agree it should at least get treated as a container, and list its elements - just noting that your screenshot example used a normal mini-widget in there.

The input widgets are already mini-widgets, but I'm not talking specifically about them here, but about the CustomWidgetBase, which can be used as a container to both "regular" mini-widgets as well as input mini-widgets.

@ArturoManzoli
Copy link
Contributor Author

ArturoManzoli commented Jan 23, 2025

On last push:

  • Mini widgets inside CustomWidgetBase and MiniWidgetBars are now listed on the left menu and has cog and trash icons functional;
  • When hovering a mini widget that is inside a CustomWidgetBase, the highlight isn't shown on the left menu. Other way around is normal and Mini widgets inside MiniWidgetBars are highlighting normally. (will be addressed on a separate issue)
Screenshare.-.2025-01-23.9_22_16.AM.mp4

@ArturoManzoli ArturoManzoli force-pushed the 1531-mini-widgets-outside-header-footer-arent-accessible branch 3 times, most recently from 4c5d196 to a5ef5c4 Compare January 23, 2025 12:40
</div>
<v-divider vertical class="opacity-10 mr-1" />
<div
class="icon-btn mdi mdi-cog"
Copy link
Member

Choose a reason for hiding this comment

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

I think you forgot the new enable/disable cog logic here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines -275 to +280
const floatingWidgetContainers = currentView.value.widgets
floatingWidgetContainers.value = currentView.value.widgets
.filter((w) => w.component === WidgetType.MiniWidgetsBar)
.filter((w) => w.options && w.options.miniWidgetsContainer)
.map((w) => w.options.miniWidgetsContainer)
return [...fixedBarContainers, ...viewBarContainers, ...floatingWidgetContainers]
return [...fixedBarContainers, ...viewBarContainers, ...floatingWidgetContainers.value]
Copy link
Member

Choose a reason for hiding this comment

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

Those line changes do not seen to have any effect in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

floatingWidgetContainers variable declaration was moved outside the const miniWidgetContainersInCurrentView computed variable.

This was changed so it will be possible to access the value from floatingWidgetContainers outside the scope of this function.
Check the change on line 649

@@ -897,5 +917,6 @@ export const useWidgetManagerStore = defineStore('widget-manager', () => {
editWidgetByHash,
setMiniWidgetLastValue,
getMiniWidgetLastValue,
customWidgetContainers,
Copy link
Member

Choose a reason for hiding this comment

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

This was added but is not being used anywhere?

Copy link
Contributor Author

@ArturoManzoli ArturoManzoli Jan 23, 2025

Choose a reason for hiding this comment

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

You are right. This was exposed to be used as part of a different approach to the solution, but wasn't used outside the store.

Will be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 635 to 655
return allContainers.some((container) => container.widgets.some((widget) => widget.hash === miniWidgetHash))
return allContainers.some((container) =>
container.widgets.some((widget: MiniWidget) => widget.hash === miniWidgetHash)
)
}
Copy link
Member

@rafaellehmkuhl rafaellehmkuhl Jan 23, 2025

Choose a reason for hiding this comment

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

Those lines don't change the code at all. Typescript is already aware that the allContainers members are of the MiniWidget type. The typing here is redundant.

You can check that by installing a Typescript language-processor in your IDE and hovering the widget arguments of the function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats because the customWidgetContainers.value that was added just above it didn't have a type on it.
I'll type it accordingly to prevent the changes you pointed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@ArturoManzoli ArturoManzoli force-pushed the 1531-mini-widgets-outside-header-footer-arent-accessible branch from a5ef5c4 to dbfd9cb Compare January 23, 2025 18:06
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.

Input widgets are not showing in the custom-widget-base.

image

@ArturoManzoli ArturoManzoli force-pushed the 1531-mini-widgets-outside-header-footer-arent-accessible branch from dbfd9cb to 07196cb Compare January 23, 2025 22:18
@ArturoManzoli
Copy link
Contributor Author

On last push:

  • Now each custom widget container has its own expansible panel;
  • All widgets types are shown;

image

@ArturoManzoli ArturoManzoli force-pushed the 1531-mini-widgets-outside-header-footer-arent-accessible branch from 07196cb to 6522298 Compare January 23, 2025 22:44
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.

Functionality works well for both container types :-)

A couple of small aesthetic suggestions, for clarity and consistency with the rest of the interface.

src/components/EditMenu.vue Outdated Show resolved Hide resolved
src/components/EditMenu.vue Outdated Show resolved Hide resolved
@ArturoManzoli ArturoManzoli force-pushed the 1531-mini-widgets-outside-header-footer-arent-accessible branch from 6522298 to e3c75b3 Compare January 24, 2025 11:09
@ArturoManzoli ArturoManzoli merged commit f105fd8 into bluerobotics:master Jan 24, 2025
11 checks passed
@rafaellehmkuhl rafaellehmkuhl changed the title Add a panel to list and edit mini-widgets inside custom mini-widgets containers Show in the edit-menu the mini-widgets that are out of the top/bottom bar Jan 29, 2025
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.

bug: mini-widgets outside header/footer aren't accessible in Edit Mode
3 participants