mWater / mwater-forms

Forms controls for mWater
GNU Lesser General Public License v3.0
3 stars 5 forks source link

adding calculation type columns in response view #214

Closed broncha closed 5 years ago

broncha commented 5 years ago

@grassick I am not comfortable with the changes. Please review. This is related to https://github.com/mWater/mwater-form-designer/issues/187

grassick commented 5 years ago

Hi Rajesh, Making expressions be a kind of fake answer seems to cause a lot of weirdness. e.g.

exports.isQuestion = (item) ->
return item._type? and (item._type.match(/Question$/) or item._type in ["TextColumn", "Calculation"] )

There might be side-effects for considering a non-question to be a question. What about creating a new render function for expressions rather than modifying renderAnswer?

broncha commented 5 years ago

I thought of

exports.isExpressionQuestion = (item) ->
  return item._type? and item._type in ["TextColumn", "Calculation"]

This would mean we duplicate the logic for renderQuestion too

since we are doing

if formUtils.isQuestion(item)
      return @renderQuestion(item, dataId)

We do need the question(header) to be rendered as it is being rendered right now. Also, this is just in the display component, would it have such side effects? This change is just required so it is visible in web and in pdf

grassick commented 5 years ago

Right. The logic to display an expression is much simpler than to display a normal question, and that includes the renderQuestion logic (doesn't have associated data or value, etc). I'd go with the approach of a separate renderExpression, since it's really not a question and there could be all sorts of impacts in other places to extend the definition of question. I'd call it "isExpression" rather than isExpressionQuestion to make the difference clear.

broncha commented 5 years ago

all done

grassick commented 5 years ago

I see you did formUtils.d.ts, but don't forget formDesign.d.ts!

grassick commented 5 years ago

Looks good to me!