Problem/Motivation

If value type is none, still a value label can be provided.

This does not seem to make sense.

Proposed resolution

Disable value label if a user selects unknown value type or if the default value type is undefined.

Remaining tasks

Check if there is any sensor that does not fit this definition.

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#6 hide_label-2560575-6.patch2.55 KBgiancarlosotelo
#6 interdiff-4-6.txt1.43 KBgiancarlosotelo
#4 hide_label-2560575-4.patch2.53 KBgiancarlosotelo
#2 hide_label-2560575-2.patch1.67 KBgiancarlosotelo

Comments

miro_dietiker created an issue. See original summary.

giancarlosotelo’s picture

giancarlosotelo commented
Status: Active » Needs review
StatusFileSize
new hide_label-2560575-2.patch1.67 KB

Providing a patch.

mbovan’s picture

mbovan commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Maybe we can use states, but this works fine too. :)

I would add tests to assert that the label field is actually hidden if the value type is 'no_value' or displayed in other case.

giancarlosotelo’s picture

giancarlosotelo commented
Status: Needs work » Needs review
StatusFileSize
new hide_label-2560575-4.patch2.53 KB

Added test.

Test also could be added to a sensor creation but after #2560213: Make configurable value type optional is commited all addable sensors are going to have this option as non configurable so I am testing this with an installed sensor which has an number as expected value.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

"all addable sensors are going to have this option as non configurable"
No at least the config sensor is addable and still will have a value type.

giancarlosotelo’s picture

giancarlosotelo commented
StatusFileSize
new interdiff-4-6.txt1.43 KB
new hide_label-2560575-6.patch2.55 KB

Oh yes, I forget that sensor :/, so now I am adding the test to the config value sensor creation.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Fixed

Yay that's quick progress! :-)

+++ b/src/Form/SensorForm.php
@@ -172,13 +173,15 @@ class SensorForm extends EntityForm {
+      if (isset($value_type) && strcmp($value_type, 'no_value')) {
+        $form['plugin_container']['value_label'] = array(

Not allowed. We only hide items with #access or their dependency is not met. Otherwise persisted values possibly are dropped on save.

Fixed and committed. We have a nice UI now, Wo0t!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

The last submitted patch, 2: hide_label-2560575-2.patch, failed testing.

The last submitted patch, 4: hide_label-2560575-4.patch, failed testing.

Status: Closed (fixed) » Needs work

The last submitted patch, 6: hide_label-2560575-6.patch, failed testing.

juanse254’s picture

juanse254 commented
Status: Needs work » Closed (fixed)

The last submitted patch, 2: hide_label-2560575-2.patch, failed testing.

The last submitted patch, 4: hide_label-2560575-4.patch, failed testing.

Status: Closed (fixed) » Needs work

The last submitted patch, 6: hide_label-2560575-6.patch, failed testing.

giancarlosotelo’s picture

giancarlosotelo commented
Status: Needs work » Closed (fixed)