-
Notifications
You must be signed in to change notification settings - Fork 24
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
Fix inability to change the text of a Label input widget #1556
Fix inability to change the text of a Label input widget #1556
Conversation
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 does make the text changeable - nice! 😃
Can you explain the purpose of the Action URL parameter though? As far as I understand it the label is a fixed value during operation, and needing to edit it with edit mode seems equivalent to just editing the Action itself to use the relevant string.
If it's supposed to also serve as a solution to #1544 then it should be editable, but it seems more intuitive to me to keep that as a separate widget (e.g. a label is a displayed descriptor that the operator doesn't change, whereas an entry is an input). If it's just static text then I think the Action URL section should be removed.
You're right, this should be removed. It has a purpose, but it's not relevant to the current state of the system. |
Ok, but wouldn’t that be better to access via a template string as part of the label text? The listener(s) could be auto-registered when the label text is modified to include one or more such templates, and then it’s possible to have text around the variable value, instead of needing separate labels for that :-) Displaying a single variable’s value seems like a job for the Very Generic Indicator. A Label widget seems more useful for static text and combinations of values. That said, we could conceivably combine those use-cases with a powerful enough string formatting language, if we decide that’s a worthwhile pursuit, but at that point it’s pretty much an equivalent of a custom mini-widget 🤷🏼♂️ |
7baa54a
to
aea22c1
Compare
Latest push: Now Label component is flagged as not having a Cockpit Action variable. *** Test with a fresh label component |
As they are now, VGIs are designed to display data using a complete and user-friendly combination of a mini-widget container, numeric value, label, and icons. |
src/stores/widgetManager.ts
Outdated
widget.options = { | ||
newWidget.options = { |
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 already fixed in the other PR, right? In this case, this one should be rebased over master to include those changes.
src/types/widgets.ts
Outdated
variableType: 'string' | 'boolean' | 'number' | ||
/** | ||
* Action parameter | ||
*/ | ||
dataLakeVariable: DataLakeVariable | ||
variableType: 'string' | 'boolean' | 'number' | '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.
We should use DataLakeVariable | null
here to preserve type checking.
@@ -68,25 +66,9 @@ onMounted(() => { | |||
color: '#FFFFFF', | |||
align: 'center', | |||
}, | |||
variableType: 'string', | |||
dataLakeVariable: undefined, | |||
variableType: '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.
The language offers us null
, which states that the value was explicitly nulled. It fits perfectly here.
@@ -361,6 +361,7 @@ const dataLakeVariableError = ref<string[]>([]) | |||
watch( | |||
() => widgetStore.elementToShowOnDrawer, | |||
(newValue) => { | |||
console.log('🚀 ~ newValue:', newValue) |
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 dev test log should be removed.
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.
Ooops
if (props.miniWidget.options.dataLakeVariable) { | ||
listenerId = listenDataLakeVariable(props.miniWidget.options.dataLakeVariable?.name, (value) => { | ||
miniWidget.value.options.layout.text = value as string | ||
}) | ||
miniWidget.value.options.layout.text = widgetStore.getMiniWidgetLastValue(miniWidget.value.hash) as string | ||
} | ||
}) | ||
|
||
onUnmounted(() => { | ||
if (miniWidget.value.options.dataLakeVariable) { | ||
deleteDataLakeVariable(miniWidget.value.options.dataLakeVariable.id) | ||
if (listenerId) { | ||
unlistenDataLakeVariable(miniWidget.value.options.dataLakeVariable.name, listenerId) | ||
} | ||
} |
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.
Didn't understand why this block of code was removed. Isn't the Label component supposed to follow the value of a data-lake variable?
Edit: I just read the conversations, but I don't follow. If the label is static, it shouldn't be an Input Widget, right? In case so, it should be renamed to Static Label so it does not confuse users.
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.
@rafaellehmkuhl my understanding is it is currently static, but in future will be able to display one or more datalake variables, e.g. as a template string: "Custom: {{ datalake.custom_var }} ({{ datalake.other_var }})".
That said, a label never directly accepts input, so I don't think it should be considered as an Input Widget (but I've also generally mentioned in other discussions that Input Widgets seem like they'd make more sense as just a sub-field of Mini Widgets, and should be usable in all the same places, but that seems an overly broad scope change for this PR).
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.
That's pretty much it.
Since we're not using this part right now, it should be removed.
aea22c1
to
372e25c
Compare
src/types/widgets.ts
Outdated
@@ -186,7 +186,7 @@ export type CustomWidgetElementOptions = { | |||
/** | |||
* Variable type | |||
*/ | |||
variableType: 'string' | 'boolean' | 'number' | |||
variableType: 'string' | 'boolean' | 'number' | null |
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.
Instead of replicating this same type over and over, could we export the 'string' | 'boolean' | 'number'
from the data-lake file and import it 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.
Signed-off-by: Arturo Manzoli <[email protected]>
372e25c
to
d443649
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.
I haven't checked the code, but the functionality seems to work as expected - nice! :-)
Fixes #1528