Skip to content

Conversation

@davidkellerman
Copy link
Contributor

Added admin- prefix to admin-theme-default/partials/head.ejs, admin-theme-default/partials/nav.ejs, and admin-theme-default/partials/scripts.ejs. Changed references to render() instead of include().

Changed AdminController.js to use generic names for element icons (rather than Font Awesome classes), then map them to class names in admin-theme-default/partials/admin-nav-menu.ejs. This leaves the icon option dangling, so added a hack to pass it as an override property for submenu items, then pick it up in admin-nav-menu.ejs.

davidkellerman and others added 4 commits September 11, 2018 00:48
Added `admin-` prefix to `admin-theme-default/partials/head.ejs`,
`admin-theme-default/partials/nav.ejs`, and
`admin-theme-default/partials/scripts.ejs`. Changed references to
`render()` instead of `include()`.

Changed `AdminController.js` to use generic names for element
icons (rather than Font Awesome classes), then map them to class
names in `admin-theme-default/partials/admin-nav-menu.ejs`. This
leaves the `icon` option dangling, so added a hack to pass it as
an `override` property for submenu items, then pick it up in
`admin-nav-menu.ejs`.
@loppear
Copy link
Contributor

loppear commented Sep 11, 2018

Tested locally and made some changes, do these work for you @davidkellerman ?

  1. Needed to register the partials dir as templates for the render calls to work
  2. Changed the icon mapping to be a class property with ability to pass in overrides in opts, rather than the one override for legacy icon opt. Added the icon mapping lookup to a few missing places.

@davidkellerman
Copy link
Contributor Author

@loppear Glad you caught the template dir registration. I "tested" the changes with my 100mhl code, rather than adding tests to the test suite.

I'm torn on the icon changes. Definitely catches the cases I left dangling. I really don't like that AdminController "knows" about the Font Awesome classes. In practical cases, overriding iconClasses isn't the whole story – you need to jigger the .ejs files to get the right resources loaded (e.g. other icon fonts, css) and probably tweak the html that's generated. You end up with an iconClasses patch and .ejs changes, stuff spread all over.

Maybe a topic for a weekly/monthly meeting? I feel like our templating is broken; it's ugly to customize or move to new infrastructure. Gut feel is that it's too "low-level," and a layer of abstraction would be useful.

@loppear
Copy link
Contributor

loppear commented Sep 11, 2018

Agree on torn / wrong level of abstraction, just didn't feel like the 'implementation in an ejs partial with a legacy parameter' addressed it either - because of the larger issue you raise

@davidkellerman
Copy link
Contributor Author

Me neither. I would have thrown it out entirely, but was afraid code somewhere would break.

@loppear
Copy link
Contributor

loppear commented Sep 11, 2018

Well, I approve this PR whether or not you revert e2fa543 and 045b5d6 then.

Seems like this needs more thought. See Issue #29 for a discussion of
potential improvements to the admin module.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants