Upgrade to font-awesome 4 without breaking compatibility with older CKANs (FA 3) #51
Conversation
| @@ -2,7 +2,7 @@ | |||
|
|
|||
| {% block page_primary_action %} | |||
| {% if h.check_access('ckanext_pages_update', {}) %} | |||
| {% link_for _('Add Article'), controller='ckanext.pages.controller:PagesController', action='blog_edit', page='', class_='btn btn-primary', icon='plus-sign-alt' %} | |||
| {% link_for _('Add Article'), controller='ckanext.pages.controller:PagesController', action='blog_edit', page='', class_='btn btn-primary', icon='plus-square' %} | |||
There was a problem hiding this comment.
I can see how the changes of the classes in the HTML are backwards compatible (since they only add classes), but I'm not so sure about these changes here which replace the old icon name in the helper call. Wouldn't this break things on old CKAN versions?
There was a problem hiding this comment.
!! You're right, of course. Hmm. All I can say is those places where it happens are primary actions and will manifest as buttons on standard CKANs. Hence the icon is less critical than in other places where we lose the view of the link completely.
There was a problem hiding this comment.
In that case I'd prefer to put these changes into version-dependent conditionals.
There was a problem hiding this comment.
Done, see below
There was a problem hiding this comment.
Great, @mattfullerton! I have one more little request (see comment), otherwise this looks good!
ckanext/pages/plugin.py
Outdated
| @@ -124,7 +134,8 @@ def get_helpers(self): | |||
| 'build_nav_main': build_pages_nav_main, | |||
| 'render_content': render_content, | |||
| 'get_wysiwyg_editor': get_wysiwyg_editor, | |||
| 'get_recent_blog_posts': get_recent_blog_posts | |||
| 'get_recent_blog_posts': get_recent_blog_posts, | |||
| 'get_plus_icon': get_plus_icon | |||
There was a problem hiding this comment.
The names of helper functions in extensions should include the extension name as a prefix to avoid name clashes. So this should be pages_get_plus_icon instead.
|
Thanks, @mattfullerton! |
Some FA 4 icon names had already been added in #51, but others were missed.
Some FA 4 icon names had already been added in #51, but others were missed.
Some FA 4 icon names had already been added in #51, but others were missed.
No description provided.