Theme feature implementation issue

[olivierdalang]

Hi,

The way the new theming feature is implemented is going to cause a lot of trouble.

Firstly, two concepts are mixed up : 1/ The ability to customize some elements of the homepage (e.g. change the jumbotron message, contact information, sponsor logos, etc.) 2/ The ability to enable/disable specific templates by the user.

But most importantly, the implementation looks extremely fragile to me :

• overwriting source files (static or templates) dynamically creates a huge mess, because it's basically modifying the code itself
• I think it's very unexpected to have collectstatic called from the code, and am pretty sure there's quite a few scenarios where this could cause a lot of trouble (e.g. when static files are hosted on a CDN).
• static files (such as css) should be served with a relatively long cache header (since they are supposedly... static !), so that changing a theme would require all users to empty their cache.

Also, I completely fail to understand how this approach is better than regular overriding Django templates/staticfiles, since anyway you need to add files to the filesystem to add new themes.

I suggest reverting completely that feature an rethink it's implementation.

1/ Should be very simple, as it's just a matter of storing some values in the database, and display them in the template (more or less Django 101). I think this is definitely a very useful feature, as it removes the need for a web developer to just make the homepage fit a project.

2/ Is probably more complex, and the need for it besides regular Django template/staticfiles overriding should be reconsidered. (also see https://github.com/GeoNode/geonode/issues/3520 which may make this slightly easier). If we really feel it's worth it (I don't), we should probably check existing Django theming apps to make sure we don't reinvent the wheel.

Cheers,

Olivier

[t-book]

1/ ... I think this is definitely a very useful feature

2/ ... If we really feel it's worth it (I don't), we should probably check existing Django theming apps to make sure we don't reinvent the wheel.

+1

[afabiani]

-1

@olivierdalang I do not agree with the sentences:

1. Using embedded theming is not mandatory and it should be used only if not using a custom GeoNode project

2. Putting variables on the templates from the database would be even worse than having a fragile solution based on CSS

The concept here is exactly to avoid filling the templates with a lot of hardcoded variables. I would suggest otherwise to make it an extension instead and enable it only if/when needed as a compromise, but I am completely against putting db variables on the templates.

[davekennewell]

Hi folks,

I'm a little lost as to what the proposed changes are here... is there a GNIP or PR you can share that would help me understand what is being proposed?

thanks! Dave

[t-book]

Hey @davekennewell Have a look at Django admin in master branch, there you find a new option for theming. One can easily change colors, the jumbotron etc..

@all: As expressed above In my opinion the option to easily change the logo or some colors without touching the template is great and will be of big help for users. Personally this does it for me and I would not need the ability of different themes. ( This does not say it cannot be useful for others, just my two cent.) Unfortuantely I cannot say something regarding the technical aspects.

[olivierdalang]

@davekennewell I was referring to the code under geonode/client (mostly introduced mostly by f46c398efe8fe68eebddc93939151b5fe69abab2 following GNIP 3743).

Please note that I very much support the original GNIP as being able to customize the content of the home page as well as some basic colors is really useful.

About 1/ Customize the home page content :

I am completely against putting db variables on the templates

But that's how every dynamic website out there works !! What is it specifically that you don't like in such a solution ?

About 2/ The ability to enable/disable specific templates by the user :

Now I'm not sure I understand correctly the goal. From some of your explanations on gitter some time ago, I thought the idea was to have the ability to switch templates as well (not only content), "à la wordpress", which was described as a future functionality in the GNIP.

If that's the case, I was just asking about how this would be better than just regular django template overriding, since both methods ultimately need to access to the file system.

If that's not the case, the implementation definitely could be made simpler.

[afabiani]

@olivierdalang

But that's how every dynamic website out there works !! What is it specifically that you don't like in such a solution ?

Not at all. Modern CMS uses well organized structures of html+css for themes and absolutely avoid to bend DB variables to the dynamic code. Remember we here are speaking about layout not contents. This is the goal of that GNIP.

[olivierdalang]

Remember we here are speaking about layout not contents

I feel there's a big misunderstanding... Isn't it mostly about content ? (changing the logo, background image, contact text, sponsors...)

Modern CMS uses well organized structures of html+css

Those which work like that still do not overwrite source code files ! How is it supposed to work when you update GeoNode ? Or if the lib is shared by several instances ? Must we now also backup the source code besides the DB and media folder ?

I still don't see the problem with having DB content in the template. If it's about the few ms of the query, we can just cache the view. If someone wants to override the homepage and not use some variables, he can. So I really fail to see the problem.

I think I made my point, maybe we need some other people's inputs, as I'm not sure we will agree :-) I'll see if I find sometime next week for a PR with what I'd see as a simpler implementation.

[olivierdalang]

What I'm talking about looks like this (did this a bit quickly today, it's mostly to show the idea, not well tested yet): https://github.com/GeoNode/geonode/commit/0a2dedf32286f6145a532cd0e533d171699b9653

Differences :

• Uses Django template tags to have dynamic content rather than overwriting template files (the enabled theme is loaded in a context_processor as we need it for every page - I even bothered to cache the results to avoid additional db queries)
• Doesn't require dynamic overwriting of external static CSS files, but instead uses CSS's cascading (!!) principle by having dynamic properties in the html directly
• Doesn't use CSS's !important! attribute (which really should avoided)
• Simplifies the enabling/disabling mecanism by using post_save signals rather than admin functions

Notes :

• The structure (css and html) is as close as possible to initial implementation, but there's room for improvement on that side as well (html semantic...)
• There were differences between themed homepage and normal homepage (e.g. the search box is in the jumbotron in the themed homepage), I didn't really solve those artistic questions so far (just put the site description in the same box than search box)
• I didn't understand what javascript clients related files (such as geonode/client/templates/geoext/layers/layer_geoext_map.html) had to do with this feature, so I just removed them (as they seemed unused by the code). My intention was not to remove any feature, so we can re-add this if removing them causes a problem.

If you like this implementation better, here's what is still to do :

• Make the categories dynamic as well (take from DB rather than hardcoded, most of the portals don't use all of the categories), and probably also some other elements (call to action button link, etc.)
• Move the static CSS bits (for partners, contact, copyright...) to regular external CSS
• Improve display of site description
• Improve admin (e.g. order/description of fields, etc.)
• Update tests
• Document

If you don't like it, I really need to understand what the problem is. As said, I see too many problems in the current implementation and would really like to have this changed before next release.

[afabiani]

@olivierdalang can you please assign a label to the issue?

[olivierdalang]

I changed to blocker, as I think we need to address this before next release (even if the decision is we don't want to fix)

[giohappy]

@olivierdalang I gave a look at your WIP and I understand that you're proposing a more "classical" approach. We will test it and, in case, it will be merged.

Anyway I think a middle way should be found between your "static" approach (any further customization must be included in the base templates conditional blocks, less flexible extension mechanism for third party modules, etc.) and the "extreme" approach by @afabiani, which gives the highest flexibility at the cost of a less "clean" extension mechanism.

[olivierdalang]

@giohappy Thanks for the review. Do you have an idea of what that middle way would be ? IMO it's already possible with normal django template inheritance to have highly flexible customization by apps (as examples, look at django-grapelli or any other django admin theme).

[olivierdalang]

Closed by #3910