Escape the optgroup labels during initialization step. #969
Conversation
| @@ -15,7 +15,7 @@ class SelectParser | |||
| @parsed.push | |||
| array_index: group_position | |||
| group: true | |||
| label: group.label | |||
| label: AbstractChosen.escapeExpression(group.label) | |||
There was a problem hiding this comment.
Making SelectParser call AbstractChosen for the escaping looks weird to me. Why not defining the method in the SelectParser instead ?
There was a problem hiding this comment.
Okay so assume that you need to escape from somewhere else as well. We certainly do escaping in the prototype & jQuery specific classes. Now will that helper function be named SelectParser.escapeExpression or AbstractChosen.escapeExpression, I don't like neither but would find the second one to be better. Perhaps Chosen.escapeExpression would be the best.
There was a problem hiding this comment.
currently, Chosen extends AbstractChosen and uses SelectParser. So IMO, SelectParser should nto depend on Chosen or AbstractChosen.
Thus, defining it directly in chosen is even worse than AbstractChosen as it would require duplicating it
There was a problem hiding this comment.
Whether it winds up in SelectParser or AbstractChosen, I find it a little awkward. Between the two, I prefer it live in SelectParser. I wonder if we should consider introducing a third option instead - something like ChosenUtils. How does that sit with you @zsombor?
@pfiller here is the fix for the master branch. Will have to create one other for the harvest_experimental branch, just in case someone will overwrite the fixes we have in the harvestapp repo.