[GitHub] [brooklyn-ui] ahgittin commented on issue #129: UI for defining parameters

[GitBox <gi...@apache.org>]

ahgittin commented on issue #129: UI for defining parameters
URL: https://github.com/apache/brooklyn-ui/pull/129#issuecomment-478653794
 
 
   great points @tbouron 
   
   > `==` vs `===` and `!=` vs `!==` ...
   
   you are 100% correct we should use the latter.  i revert to bad other language habits.  TY.
   
   > (3) is absolutely critical if you have duplicate key. We can live with awkward behaviour but not breaking the UI. This can be solved by adding validation on the key field, to check if any exists already.
   
   yeah, i buy that.  i don't see an easy way to do it with validation for the case where duplicate keys are set in yaml.  (we already did it if you tried to supply duplicate keys in the ui so i think from yaml is the only way.)  what i've done instead is that on model load if we find a duplicate we append a number to it, so `brooklyn.parameters: [ { name: y, type: string }, { name: y, type: string }, { name: y, type: string } ]` becomes `y` `y2` `y3`, with warnings logged to the console.  it's awkward and surprising but at least doesn't break the ui.  we could have yaml model validation errors as well for this though the problem would still manifest in the graphical composer if invalid yaml was passed in so we need a way to handle it here.  the other option would be to refuse to show parameters.
   
   > any constraints added into the the constraint field don't work.  Worse, the value is cleared as soon as I unfocus the field which is very irritating if the constraint is long. And setting the constraints in either JSON mode or directly in YAML will silently remove them.
   
   if it's valid JSON it works, but it must be e.g. `["required"]` -- -have changed message to note it needs the brackets _and_ double quotes.  could improve by supporting yaml parsing or a dedicated editor but going for minimal working capability here.  also agree clearing on unfocus is awkward but of course there's a lot of work to store invalid models if we did something else.  we could maybe forbid the unfocus.
   
   > the change to have a single filter/add field is moot. It's awkward to use and hard to understand what it's doing. This would be high priority to fix IMO but in the sake of getting this PR merged (and the spirit of my first point) it can be addressed in a subsequent PR.
   
   doesn't moot mean "no longer relevant or not worth discussion"?  i think you mean it's a step backwards.  personally i thought having two was awkward and one is fairly self-explanatory.  happy to see further evolution and testing but agree it's such a minor point as to be _moot_ :).
   
   > re the new control for filter/add config and parameter, I think the icons you added are unnecessary in this case: it takes valuable real estate and don't give meaning. For example, the cross for required will be interpreted as "close" or "remove" the required params. I think i would be best to not include them for now, until we find a better way.
   
   i removed checkboxes so in total we have a real estate savings.  i think icons make it look more polished, but agree the `x` is confusing.  have changed it to a `stop-circle` (bit white square in black circle).  again happy to see further evolution but i'd like to keep the icons.  if you feel strongly let's get a tie-break opinion!
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services