-
Notifications
You must be signed in to change notification settings - Fork 22
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
Very generic input: Custom widget creator #1418
base: master
Are you sure you want to change the base?
Very generic input: Custom widget creator #1418
Conversation
8cb8e98
to
96ab58a
Compare
Last push: Fix dropdown selector not keeping last selected option after reload |
96ab58a
to
5cca995
Compare
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.
- The horizontal edit-menu bar with the custom widget options does not have a consistent scroll with respect to the other two menus (regular and mini).
Testes on macOS. Vertically scrolling the bar should scroll horizontally.
- This is breaking the edit-menu widget container width again:
-
Does any problem arise if we allow the user to add the custom input widgets to the mini-widget containers? This would make them ever more powerful! This way they can have them on the top/bottom bar.
-
Should we have this as a button? Is there any other usage that is not tied to a Cockpit Action Variable?
- The slider on "large" size is overflowing the container and the label as well. Increasing the container size does not help:
- The changes are using "Cockpit Action Parameter" instead of the new name "Cockpit Action Variable".
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.
Question: should we indeed separate the whole pipeline for the custom widgets from the mini-widgets?
I understand having them on a separate dropdown on the edit-menu, to make it easier for the user to understand, but I couldn't see a good reason to separate their container from the mini-widgets. Being able to mix them feels like a big feature to me, and if we separate them now, it will be difficult to merge them later.
src/assets/defaults.ts
Outdated
everMounted: false, | ||
configMenuOpen: false, | ||
highlighted: false, | ||
customWidgetVars: CustomWidgetVarsType.Button, |
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 call it customWidgetType
?
src/components/EditMenu.vue
Outdated
:ref="(el) => (miniWidgetContainers[miniWidget.component] = el as HTMLElement)" | ||
:key="miniWidget.hash" | ||
class="flex flex-col items-center justify-between w-full rounded-md bg-[#273842] hover:brightness-125 h-[90%] aspect-square cursor-pointer elevation-4 overflow-clip pointer-events-none" | ||
class="flex flex-col items-center justify-between rounded-md bg-[#273842] hover:brightness-125 h-[90%] aspect-square cursor-pointer elevation-4 overflow-clip pointer-events-none" |
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.
This is the part that breaks the auto-width.
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.
Yes, it is and it's weird how this zombie change keep being deleted and brought back to life over and over... 😄
src/components/EditMenu.vue
Outdated
<div class="flex items-center justify-center w-full p-1 transition-all bg-[#3B78A8] rounded-b-md text-white"> | ||
<span class="whitespace-normal text-center">{{ | ||
element.name.replace(/([a-z])([A-Z])/g, '$1 $2').replace(/^./, (str) => str.toUpperCase()) || | ||
'Very Generic Indicator' |
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.
Is this right?
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.
This code is turning a camelCase name such as RelativeAltitudeIndicator to Relative Altitude Indicator to fit the card title. But the exception is indeed unnecessary.
src/components/ElementHugger.vue
Outdated
type ElementType = { | ||
/** | ||
* | ||
*/ | ||
position: { | ||
/** | ||
* | ||
*/ | ||
x: number | ||
/** | ||
* | ||
*/ | ||
y: number | ||
} | ||
/** | ||
* | ||
*/ | ||
size: { | ||
/** | ||
* | ||
*/ | ||
width: number | ||
/** | ||
* | ||
*/ | ||
height: 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.
Blank jsdocs.
src/components/EditMenu.vue
Outdated
<div | ||
v-show="widgetMode === 'Regular widgets'" | ||
class="2xl:text-md text-sm mt-3 2xl:px-3 px-2 bg-[#3B78A8] rounded-lg" | ||
> | ||
Click or drag to add | ||
<div v-show="widgetMode === 'Regular'" class="2xl:text-md text-sm mt-3 2xl:px-3 px-2 rounded-lg"> | ||
(Click on card to add) |
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 are wrongly removing a functionality here?
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 did rename the selector so it wont have the word 'widgets' repeated over and over. But the helper text should have remained the same.
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 mean the "Click or drag to add" vs "Click to add".
src/components/WidgetHugger.vue
Outdated
width: `${100 * size.value.width}%`, | ||
height: `${100 * size.value.height}%`, | ||
width: disableResponsiveness.value ? `${1000 * size.value.width}px` : `${100 * size.value.width}%`, | ||
height: disableResponsiveness.value ? `${1000 * size.value.height}px` : `${100 * size.value.height}%`, |
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.
Curiosity: what was the motive for that the custom widgets to be fundamentally different in the resize behavior? I found it to be counterintuitive to the user.
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.
There was a concern about how the various input mini-widgets would react to the custom-widget-base responsiveness. Generic widgets shrink with the window size and its contents are scaled down too.
But this shrink behavior wasn't ideal for input components.
This prop was also causing some size issues on a recently added widget base.
I'll remove it for now and let's see how the responsiveness is doing on the custom widgets.
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 you forgot to remove this (and the other mentions to disableResponsiveness
along the code).
Totally agree. Allowing the user to add custom mini-widgets to the top, bottom bar and to any other future uses for mini widgets will indeed make Cockpit a lot more powerful. |
Can you give more info about it? Maybe an example of another usage so we decide if we should implement now or on the next functionalities update |
798d9c5
to
c53b578
Compare
On last push:
Screenshare.-.2024-10-30.9_57_31.AM.mp4 |
My point is that all input widgets are being created to inject variables in the data-lake, and having a button there means creating this variable is optional, when it's in fact mandatory. Those should be required fields. |
I'm not able to config the input widgets with the last PR. It was working fine in the previous version thought: Kapture.2024-11-01.at.06.42.32.mp4 |
This is something related to the instantiation of the input widget on the top and bottom bar. If you refresh the screen the config panel will open normally. Will fix that |
I understand you point that the variable name is mandatory to inject data, but if the user is, at the moment, just creating the custom widget and arranging its layout? Maybe it doesn't need to be named/registered at that very moment, and the user (or even someone else) can do this data injection config on a later moment. The blue 'create' button was thought the be spotted right away by the user and probably wont be missed out. Also, the whole process for controlling something by the input widgets will usually need some documentation reading/video tutorial for the user to fully understand its applications and usage. So, the variable creation will be something internalized by the user, I suppose. Also, every variable injected on the data lake will take a small amount of system resources to be kept tracked or on listening mode. Let me know your thoughts about this. Anyway, I modified the workflow, so when an input widget is placed on the top or bottom bar, its config panel will always pop out. This way, the user will be 'invited' to configure its proprieties right away |
c53b578
to
a1233c5
Compare
@ArturoManzoli it does make sense! |
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'm trying to setup a button widget but it's not calling the Cockpit Action it was assigned to:
Kapture.2024-11-04.at.23.52.19.mp4
a1233c5
to
f629ea6
Compare
src/components/EditMenu.vue
Outdated
@@ -596,28 +639,52 @@ import { computed, onMounted, ref, toRefs, watch } from 'vue' | |||
import { nextTick } from 'vue' | |||
import { type UseDraggableOptions, useDraggable, VueDraggable } from 'vue-draggable-plus' | |||
|
|||
import { defaultMiniWidgetManagerVars } from '@/assets/defaults' | |||
import { defaultCustomWidgetManagerVars, defaultMiniWidgetManagerVars } from '@/assets/defaults' | |||
/* @ts-ignore */ |
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.
Those @ts-ignore
are not needed. You can remove them and push and will see that the CI will not complaint.
src/components/EditMenu.vue
Outdated
miniWidget.name.replace(/([a-z])([A-Z])/g, '$1 $2').replace(/^./, (str) => str.toUpperCase()) || | ||
'Very Generic Indicator' | ||
miniWidget.name.replace(/([a-z])([A-Z])/g, '$1 $2').replace(/^./, (str) => str.toUpperCase()) |
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.
* Mini-widget instance | ||
* | ||
*/ |
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.
No need to remove this line.
src/components/WidgetHugger.vue
Outdated
width: `${100 * size.value.width}%`, | ||
height: `${100 * size.value.height}%`, | ||
width: disableResponsiveness.value ? `${1000 * size.value.width}px` : `${100 * size.value.width}%`, | ||
height: disableResponsiveness.value ? `${1000 * size.value.height}px` : `${100 * size.value.height}%`, |
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 you forgot to remove this (and the other mentions to disableResponsiveness
along the code).
src/libs/actions/data-lake.ts
Outdated
throw new Error(`Cockpit action variable with id '${variable.id}' already exists. Update it instead.`) | ||
console.log(`Cockpit action variable with id '${variable.id}' already exists. Renaming to ${variable.name}_new.`) | ||
showSnackbar({ | ||
message: `Cockpit action variable with id '${variable.id}' already exists. Renaming to ${variable.name}_new.`, | ||
}) | ||
variable.id = `${variable.id}_new` | ||
variable.name = `${variable.name}_new` | ||
cockpitActionVariableInfo[variable.id] = variable |
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.
There's no need for the snackbar here.
You can have the snackbar in the component that is calling createCockpitActionVariable
.
This is a void method with a throw, so there you can put a try/catch block to deal with error/success case, like so:
// EditMenu.vue
import { useSnackbar } from '@/composables/snackbar'
const { showSnackbar } = useSnackbar()
try {
createCockpitActionVariable(variable)
showSnackbar({ message: 'Parameter successfully updated', variant: 'success' })
} catch(error) {
showSnackbar({message: `Failed to add Cockpit action variable with id '${variable.id}'. Name already exists. Please rename it.`})
}
If necessary to deal with the renaming process, please do it in the EditMenu.vue
component. You can do that by adding an code
property to the error (inside the data-lake.ts
file) and checking for the error in the Vue component, although I think the more correct approach is to ask the user to rename the variable, so it's clear to them that they already have a variable with that name).
@@ -312,7 +312,7 @@ | |||
</template> | |||
|
|||
<script setup lang="ts"> | |||
import { computed, onMounted, reactive, ref } from 'vue' | |||
import { computed, onMounted, reactive, ref, watch } from 'vue' |
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.
Unnecessary change. Should be removed.
@@ -665,6 +665,7 @@ const saveUrlParameter = (): void => { | |||
|
|||
onMounted(() => { | |||
loadSavedActions() | |||
console.log('🚀 ~ allSavedActionConfigs:', allSavedActionConfigs.value) |
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.
Unnecessary change. Should be removed.
f629ea6
to
9dc9b1a
Compare
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.
Code-wise I think everything is fine now.
The button is also working (for calling actions).
What I can't get to work are the inputs. I tried to add a slider input, but it isn't updating it's action variable. It keeps undefined, on both boot and while moving it. The test below has both a setInterval
as well as a listenCockpitActionVariable
added to the code, and none get any change in the slider value:
Kapture.2024-11-11.at.06.57.12.mp4
a6adc55
to
6709204
Compare
We discussed in private and came to the conclusion that there was a missing function in the data-lake for those to work as expected. The ability to set the initial value was implemented on #1443 and will be used here. |
6709204
to
fb1ce39
Compare
The modifications on the data-lake and input mini widgets are implemented. |
f84cbd5
to
eb882a2
Compare
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.
@ArturoManzoli I've found some problems in my last test. Will list them below:
- I'm testing here but I'm having the same
NaN
problem with the Dial widget I was having in our last talk. Was this part worked on?
Kapture.2024-11-19.at.08.52.24.mp4
I made sure I was using the latest branch version for the test. Vite server was reloaded and application process was killed and reset.
I could reproduce it with both old Dial widgets as well as to ones I had just added. The problem happens if you add a Dial widget, create a data-lake variable to it but reload the application without using the dial for the first time before the reload. I can reproduce it 100% of the times I try. If I use the dial (turn it) before reloading Cockpit, the problem does not happen, but if I don't, the Dial gets stuck at NaN forever.
- When I drag an input widget from the CustomWidgetBase, the trash icon does not appear.
Also around that, the edit-menu left panel is not showing the CustomWidgetBase as a container for mini-widget, so that the user can delete or configure mini-widgets from there, although this is a problem also for the current MiniWidgetBar widget, so I opened issue #1452 to track that. Feel free to work on it on this PR or to let it as it is and we fix it later for both.
- Both the switch as well as the checkbox, when with a false value, require two clicks to change to true again.
src/types/widgets.ts
Outdated
export enum CustomWidgetType { | ||
Button = 'boolean', | ||
Checkbox = 'boolean', | ||
Dial = 'number', | ||
Dropdown = 'string', | ||
Label = 'string', | ||
Slider = 'number', | ||
Switch = 'boolean', | ||
} |
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.
This was created but was never used.
src/types/widgets.ts
Outdated
/** | ||
* Variable type | ||
*/ | ||
variableType: string |
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.
Checkbox is not of type string.
Actually all elements are set as string type.
src/types/widgets.ts
Outdated
/** | ||
* Last known value of the element | ||
*/ | ||
lastValue: string | number | boolean | undefined |
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.
Last value here should be boolean or undefined, not string or number.
eb882a2
to
c722735
Compare
@rafaellehmkuhl, I had to relocate the lastValue storage location out of the mini-widget data object due to reactivity problems on some boolean vuetify components. Its working fast and reliable as I tested. Take a look and see if we need to store those values somewhere else. |
I feel like those changes are passing a lot of the responsibility that should be in the widgets themselves to the widget management store, but at the same time I don't want to postpone the merging of this feature much more. I will perform some more tests to see if everything is working and take a look at the code again, and if everything goes right we are good to go. |
src/stores/widgetManager.ts
Outdated
const isElementsPropsDrawerVisible = ref(false) | ||
const elementToShowOnDrawer = ref<CustomWidgetElement>() | ||
const widgetToEdit = ref<Widget>() | ||
const miniWidgetLastValues = useBlueOsStorage<Record<string, any>>('mini-widget-last-values', {}) |
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.
One small thing here: the storage key should start with "cockpit-...".
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.
Done. I also have relocated the save widget method to outside the store
Signed-off-by: Arturo Manzoli <[email protected]>
c722735
to
4a02d30
Compare
Signed-off-by: Arturo Manzoli <[email protected]>
4a02d30
to
a5bbb9d
Compare
Widget creation toolkit for parametric inputs and control via HTTP requests;
Screenshare.-.2024-10-18.4_09_58.PM.mp4
Screenshare.-.2024-10-18.4_15_54.PM.mp4
Closes #1291