templates and controllers

[Thomas De Schampheleire]

On Sun, Feb 22, 2015 at 4:56 PM, Mads Kiilerich <mads at kiilerich.com> wrote:
> On 02/21/2015 09:35 PM, Thomas De Schampheleire wrote:
>>
>> On Tue, Feb 17, 2015 at 4:26 AM, Mads Kiilerich <mads at kiilerich.com>
>> wrote:
>>>
>>> On 02/15/2015 09:45 PM, Thomas De Schampheleire wrote:
>>>>
>>>>
>>>> A quick search reveils:
>>>>
>>>> $ grep -rn '= render' controllers/
>>>> controllers/followers.py:56:        c.followers_data =
>>>> render('/followers/followers_data.html')
>>>> controllers/forks.py:126:        c.forks_data =
>>>> render('/forks/forks_data.html')
>>>> controllers/pullrequests.py:207:        c.pullrequest_data =
>>>> render('/pullrequests/pullrequest_data.html')
>>>> controllers/summary.py:111:                        readme_data =
>>>> renderer.render(readme.content,
>>>> controllers/admin/gists.py:234:        rendered =
>>>> render('admin/gists/edit.html')
>>>> controllers/admin/admin.py:144:        c.log_data =
>>>> render('admin/admin_log.html')
>>>> controllers/journal.py:210:        c.journal_data =
>>>> render('journal/journal_data.html')
>>>> controllers/journal.py:353:        c.journal_data =
>>>> render('journal/journal_data.html')
>>>>
>>>> Unless you see a good reason why the above is like that, this could be
>>>> cleaned up...
>>>
>>>
>>> Agreed, it seems like some cleanup could make templates simpler or
>>> "better".
>>
>> It seems that this pattern is used to be able to return data-only in
>> partial requests (those loaded via ajax/asynchtml javascript functions
>> with HTTP_X_PARTIAL_XHR set).
>>
>> In the case of pullrequests, I don't think this is actually being
>> using, but for example it is used for the journal.
>>
>> Not sure what to do with this...
>
>
> We could make it simpler and always load the data async, also in the initial
> page load. We will have less code paths to consider if we always do it the
> same way. I would prefer that.

One concern is how usable/functional Kallithea is when Javascript is
disabled in the browser.
Good practice says that the core functionality should still work, so
loading async for the initial code would not be good, unless you also
handle something in <noscript>.

Then the principle described in
http://pylons-webframework.readthedocs.org/en/latest/helpers.html#partial-updates-with-ajax
could be better: have the initial load include the data template from
pure mako, and let the pager load new pages dynamically. When
javascript is disabled, everything still works.

>
> As a simple short term improvement we could replace c.journal_data in the
> template with an include of journal_data.html .

Yes, similar to what I did in my last pullrequest series right?
There, I could get rid of the variable c.foo_data altogether as the
dynamic aspect was not used.

But in the case of journal, since dynamic loading is used, the
variable still would exist.
I think the way to solve this differently, is to have two controller
methods, one serving the full page and one the data portion. This is
how 'my pullrequests' was handled initially (before my patches).
Then you don't need a magic 'partial' variable in the HTTP request.

Is this correct reasoning?

>
> One future option could be to just return the data as json and generate the
> dom on the client side.

Yes, could be, but again I think we should be careful in relying on
Javascript for core display of data, as mentioned above.

Best regards,
Thomas