surveyjs / survey-library

Free JavaScript form builder library with integration for React, Angular, Vue, jQuery, and Knockout.
https://surveyjs.io/form-library
MIT License
4.09k stars 795 forks source link

Enabling comment and other doesn't work properly. #8019

Open SamMousa opened 5 months ago

SamMousa commented 5 months ago

Are you requesting a feature, reporting a bug or asking a question?

Bug

What is the current behavior?

image

One can enable both a comment field AND the other option in a radiogroup. However this doesn't work properly for several reasons:

What is the expected behavior?

If this configuration is not supported it should not be allowed in the creator (in which case this issue should be moved) If it is supported the library should properly support it:

How would you reproduce the current behavior (if this is a bug)?

Provide the test code and the tested page URL (if applicable)

Tested page URL: https://surveyjs.io/create-free-survey

Test code

{
 "pages": [
  {
   "name": "page1",
   "elements": [
    {
     "type": "radiogroup",
     "name": "question1",
     "showCommentArea": true,
     "choices": [
      "Item 1",
      "Item 2",
      "Item 3"
     ],
     "showOtherItem": true
    }
   ]
  }
 ]
}
JaneSjs commented 5 months ago

Hi @SamMousa, Thank you for your inquiry.

When a Radiogroup question enables both the Other and Comment options, we always save the value of the Other option within an array of selected options and the comment text as a separate item under the questionName-comment key.

We have recently discussed this issue and have come to the conclusion that we don't have many options here, considering that we have two places to save values: a question object and a "question-Comment" entry. Even if we decided to change it, we would introduce a breaking change.

Therefore, the storeOtherAsComment option is actually ignored in this usage scenario. If you wish to keep the Other option's text as a separate item and save the Comment text as a separate entry as well, you can create a separate Comment/Long Text field for the user to enter comments.

If this configuration is not supported it should not be allowed in the creator (in which case this issue should be moved)

You may wish to disable the comment option and hide corresponding properties using the creator.onShowingProperty event.

The Comment field uses label of the "Other" option

Regarding the Comment label: you can override it using the commentText property value.

{
 "pages": [
  {
   "name": "page1",
   "elements": [
    {
     "type": "radiogroup",
     "name": "question1",
     "showCommentArea": true,
     "commentText": "Your comments here",
     "choices": [
      "Item 1",
      "Item 2",
      "Item 3"
     ],
     "showOtherItem": true
    }
   ]
  }
 ]
}

I hope this clarifies the situation. Please let me know if you have further questions.

Thanks

SamMousa commented 5 months ago

Therefore, the storeOtherAsComment option is actually ignored in this usage scenario.

This is already a breaking bug. Data validation fails because of this.

Even if we decided to change it, we would introduce a breaking change.

This is not true at all. If you add a new otherSuffix option but not give it a default value you could make the new (and better) behavior opt in. Continuing to build on a bad foundation and then marking legitimate bug reports as user questions makes both the code worse as the experience of developers spending their time on creating proper bug reports.

Have you discussed my proposed solution of a setting that opts in to the cleaner behavior instead of silently ignoring settings?

andrewtelnov commented 5 months ago

@SamMousa You can make showCommentArea property invisible for select base questions: checkbox, dropdown, radiogroup, tagbox and you will get the previous behavior. Survey.Serializer.findProperty("selectbase", "showCommentArea").visible = false;

Adding new options will just make things more complicated. If you disable "showCommentArea" for this question types then you will not have this problem as well.

I think it was a mistake to make this property visible for these quesiton types at the first place. We did it several months ago because several of our users asked a lot about it. This property was invisible in previous versions.

Thank you, Andrew

SamMousa commented 5 months ago

I think it was a mistake to make this property visible for these quesiton types at the first place.

I can get behind that reasoning.

What I can't get behind though is that the default configuration of the creator is a non-functioning one. I'd argue that having separate suffixes for comment and other is an improvement. It can be done without breaking backward compatibility since you can just have the new parameter default to the current one.

Feels bad that we need a workaround to get working behavior...

andrewtelnov commented 5 months ago

@SamMousa You can achieve what you need by hiding the showCommentArea property. Introducing new property will require us to modify our Dashboard code and probably Pdf + developers will unlikely find this new settings and they will ask a question anyway. Probably it will be better if we hide showCommentArea by default for select base questions.

Thank you, Andrew

SamMousa commented 3 months ago

Probably it will be better if we hide showCommentArea by default for select base questions.

I did this and just found out that that also removes it from Ranking questions. So be sure to make an exception for ranking questions if you decide to add this.

andrewtelnov commented 3 months ago

@SamMousa You can create a copy of this property for ranking question and then change it's attributes: Survey.Serializer.getProperty("ranking", "showCommentArea").visible = true; "getProperty" will return the property from this class. If there is no property in this class, then it goes into a parent class where it defines, create a copy and return it. After that Survey.Serializer.findProperty("ranking", "showCommentArea"); returns the new created property copy from the "ranking" class.

Thank you, Andrew

SamMousa commented 3 months ago

I'm using a more generic solution:

        this.onCanShowProperty.add((sender, options) => {
            if (options.property.name === 'showCommentArea') {
                options.canShow = !options.obj.supportOther();
            }
        });

Essentially implementing your suggestion of not having the comment area option visible for any question that supports other. Feel free to close this if you don't want to implement your own suggestion:

Probably it will be better if we hide showCommentArea by default for select base questions.

andrewtelnov commented 3 months ago

@SamMousa This code will work as well.

Thank you, Andrew