CRM-20929 - Convey css classes to main page title from Angular #10711
Conversation
colemanw
commented
Jul 20, 2017
•
colemanw
commented
- CRM-20929: Allow styling of page title in Angular
ang/crmUi.js
Outdated
| newDocumentTitle = scope.crmDocumentTitle || $el.text(), | ||
| cls = $el.attr('class').split(' '), | ||
| classes = _.filter(cls, function(c) { | ||
| return c.indexOf('ng-') !== 0; |
There was a problem hiding this comment.
This seems to say "sync the CSS classes, except when you shouldn't, and here's a rule-of-thumb about when you shouldn't". How confident are you in this rule-of-thumb? It seems like it would transfer over bits for other directives. (For example, crm-page-title is replicated over -- which I didn't intuitively expect.)
I don't want to block if you feel strongly about it, but it seems like it could just as easily list the sync-able classes in different attribute with less risk of accidents, eg:
<h1 crm-page-title crm-page-class="foo bar">{{ts('Hello')}}</h1>then change $el.attr('class') to scope.crmPageClass, and you don't the filter.
There was a problem hiding this comment.
Well crm-page-title is the class I think we should add no matter what. I started to leave the PR at just that, then got inspired to add other stuff from the class attribute, then realized there was some ng-* junk in there so stripped it out.
I don't feel strongly one way or the other about your suggestion. I'm tempted to just take it back to the simpler .addClass('crm-page-title') and be done with it, since that satisfies the use case.
|
I gave some inline feedback on design, which you can take or leave. The behavior is not entirely obvious -- it should probably mentioned in the docblock. From functional perspective, this worked as described on a |
|
@totten I went with the simpler approach. The fancier stuff is YAGNI. |
|
Sounds good to me. |
