django cms integration #1

Merged
merged 13 commits into from Dec 13, 2012

Projects

None yet

3 participants

@jssevestre
Contributor

Hello

please see attached simple code to use django-terms with django-cms:

TermsProcessor
a context processor to parse all plugins output

TermsIndexPlugin
to display all terms on a page as

BR

@BertrandBordage
Owner

Excellent! I start reviewing it right now!

@BertrandBordage

Hum... I think rendered_context.replace('<br>', '<br/>') would be better.

Owner

I agree

@BertrandBordage

I prefer relative imports in this case: from .html import....

@BertrandBordage

Is there a reason for this import to be here? No offense, I am just wondering :)

Owner

No offense, i'm not an expert
I just follow/copy http://django-cms.readthedocs.org/en/latest/extending_cms/custom_plugins.html#example

It's better on top.

Yes, it's better on top :)
And you are doing pretty well!

@BertrandBordage

Same thing here: from .models import Term.
Could you also add another newline before and after the class? 'Just for PEP8 compliance.

@BertrandBordage

I don't think adding newlines between definitions is the proper way to do styling :)
Since django-cms is using sekizai_tags, you could use something like this before (to add 5px after each dd that is followed by a dt):

{% addtoblock 'css' %}
  <style>
    dl.terms-index dd+dt {
        margin-top: 5px;
    }
  </style>
{% endaddtoblock %}
Owner

may be it's better without styling in this base template.
But I will add 'terms-index' class to

so users could style

@BertrandBordage BertrandBordage and 1 other commented on an outdated diff Dec 13, 2012
terms/cms_plugin_processors.py
@@ -0,0 +1,25 @@
+#!/usr/bin/env python
+#-*- coding:utf-8 -*-
+
+from .html import TermsHTMLReconstructor
+from django.template import Context, Template
+
+
+def TermsProcessor(instance, placeholder, rendered_content, original_context):
+ """
+ This processor mark all terms in all placeholders plugins except termsplugins
+ """
+ if 'terms' in original_context:
+ return rendered_content
+ else:
@BertrandBordage
BertrandBordage Dec 13, 2012 Owner

This else statement is useless. If 'terms' in original_context is True, you return something, which also means you leave this function. The else is therefore verified each time it is parsed.

@jssevestre
jssevestre Dec 13, 2012 Contributor

ok removed!

@BertrandBordage

This line still needs to be replaced with a str.replace. Or it could even be removed, since this exception is only raised in DEBUG mode.

Owner

I replace with rendererd_content.replace
I think we can't let the exeption raise even in DEBUG mode.
it's a parsing bug, and this is a workaround !

It's not a parsing bug, it's an HTML malformation that should never happen, which is why I chose to give it this particular importance.
<br> is not the only tag that is oftenly malformed, you can also take the cases of <input> and <link>. What you are trying to make should idealy be an exhaustive list of common cases handled by TermsHTMLReconstructor. And it is not the job of django-terms to guess why someone made a mistake.

So, in my opinion, this is not where these replacements should be done, and the list would necesserally buggy (your replacement is not handling <BR> or <BR >).

This is why, in the develop branch, I chose to replace my bad HTMLReconstructor with the excellent BeautifulSoup4. It automatically fixes these common malformations. But the develop branch isn't stable yet, so you have to wait and... Set DEBUG to False or fix your HTML malformations.

Owner

OK, it's not the place to fix other app bugs !

  • HTML <br> malformation from django-cms
  • <br> HTMLValidationWarning from HTMLReconstructor

So I clean those 2 lines and wait for develop branch

@BertrandBordage

This has no effect. A Python string being immutable, this method is not modifying it in place, it returns another string.

@BertrandBordage
Owner

That's it! Do you have anything else to change, or can I merge your pull request?
Besides, I will then create an AUTHORS file containing our names.

@jssevestre
Contributor

Fine,
No more change in code right now !
May be a usage note will be usefull : just wait a moment !

@jssevestre jssevestre Usage note
for cms_plugin_processor and TermsIndexPLugin
578739b
@BertrandBordage

CMS_PLUGIN_PROCESSORS = (

@BertrandBordage

If I can remember correctly, one doesn't need to add 'app.cms_plugin``to INSTALLED_APPS in order to use its plugins.'terms'` should be enough.

@BertrandBordage

Typo: definitions.

@jssevestre
Contributor

2ea5f2a fix README review

@BertrandBordage BertrandBordage commented on the diff Dec 13, 2012
terms/cms_plugins.py
@@ -0,0 +1,19 @@
+from cms.plugin_base import CMSPluginBase
+from cms.plugin_pool import plugin_pool
+from cms.models.pluginmodel import CMSPlugin
+from django.utils.translation import ugettext_lazy as _
+from .models import Term
+
+
+class TermsIndexPlugin(CMSPluginBase):
+ model = CMSPlugin
+ name = _("Terms Index Plugin")
+ render_template = "term_plugin.html"
+
+ def render(self, context, instance, placeholder):
+ terms = Term.objects.all()
@BertrandBordage
BertrandBordage Dec 13, 2012 Owner

I just realized something: if you select every Term, you will also display those who have no definition but a link instead. So you either need to apply a .exclude(description='') to this QuerySet or handle this (not so) special case in the template.

@jssevestre
jssevestre Dec 13, 2012 Contributor

IMHO we should add link in the template, maybe a short version like

<dd>{{ term.definition|safe }} {{ term.url|urlizetrunc:15 }}</dd>

If there is a description, it will be add after. Else, it will be the only content.

@BertrandBordage
BertrandBordage Dec 13, 2012 Owner

Good. Maybe a newline this time? Only if there is a url, of course. And maybe more letters shown in the urlizetrunc. Something like:

<dd>
  {{ term.definition|safe }}
  {% if term.url %}
    <br/>
    {{ term.url|urlizetrunc:40 }}
  {% endif %}
</dd>
@jssevestre
jssevestre Dec 13, 2012 Contributor

No, if definition is empty and url not, the new line is still there.
Perhaps, but more complex :

{{ term.definition|safe }}
{% if term.url and term.definition %}<br/>{%endif %}
{{ term.url|urlizetrunc:40 }}
@BertrandBordage
BertrandBordage Dec 13, 2012 Owner

Oh, yes. Sorry. Go on, I think this will be the last one before I merge all your changes.

@BertrandBordage BertrandBordage merged commit 5f2558a into BertrandBordage:master Dec 13, 2012

1 check failed

default The Travis build failed
Details
@coveralls

Coverage Status

Changes Unknown when pulling d70125f on jssevestre:master into * on BertrandBordage:master*.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment