Skip to content
This repository has been archived by the owner on Dec 9, 2019. It is now read-only.

I added I18N support to flatblocks #10

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

I added I18N support to flatblocks #10

wants to merge 15 commits into from

Conversation

natethelen
Copy link

I went through the whole project and updated everything. I verified that all test cases work. Let me know if anything is missing. I am running this new version on our site and all seems to be working well.

@natethelen
Copy link
Author

The only issue I can think of is that it requires that either you are using the RequestContext or that you add a line to your templates once you upgrade. Probably will break some users sites until they have done this (I output a string to the page to check their logs). May be able to add some code that "ignores" the language if cannot get it, but then new users may run into a situation where it isn't working and they don't know why.

@zerok
Copy link
Owner

zerok commented Sep 12, 2011

Thank you :-)

The update path is a real issue, though, in my opinion. Is there any real reason why the language couldn't be made completely optional? Also imo the field should also be nullable to make the upgrade easier.

@natethelen
Copy link
Author

Yep, missed that. Let me mess around a bit more and see what can be done! Will ping back within a week

Fixed error happening when app is part of project during first syncdb
Added required values to test project
@natethelen
Copy link
Author

I went through the upgrade path, starting with your version and upgrading to mine, and made sure that all the errors made sense for them as well as working nicely for a new install. An upgrade requires that the user adds columns, but that seems pretty standard for module upgrades. Let me know what you think.

@zerok
Copy link
Owner

zerok commented Sep 19, 2011

Thank you :-) I will hopefully get to it this weekend or before. I also already have a patch for south integration lying around here somewhere so it might make a good mix with your changes :-)

@zerok
Copy link
Owner

zerok commented Sep 23, 2011

  • If you use LANGUAGE_CODE when creating a new instance of a flatblock, using current_language in the template isn't really an option since this will only use setting.LANGUAGE_CODE as as fallback if no real translation can be found. For this there has to be a fallback solution (preferably something on the manager level).
  • I'm kind of missing the point of LANGUAGE_CODE in the templates. It (1) breaks backwards-compatibility and (2) could be made redundant using django.utils.translation.get_language, IMO. As an option, absolutely, but not as a required context item.
  • Personally I'm not so sure about this translation approach compared to something like django-nani. I certainly prefer nani's in general but there might be some use-cases where - for flatblocks - yours might be better.
  • Because of this I'm right now more in favor of first really working on asolution for flatblocks on multiple sites. This is a rather well understood problem and solved by multiple apps out there, while language support is still kind of a battlefield.
  • But even site-support isn't that clear cut. Your solution binds a flatblock to one site, while the current implementation makes it accessible to every site. Personally, I prefer the flatpages approach with a page being possibly attached to multiple pages.
  • What is "I18L"?
  • And please run the unittests before updating a pull request :-)

@natethelen
Copy link
Author

  1. Yeah, didn't think that one through enough. I removed the reliance of LANGUAGE_CODE being in the template
  2. django-nani seems great. I hadn't seen it before now (looked though 5-6 others originally). Depends on if you want to rely on another project...
  3. i18L was a typo. Fixed.
  4. Sorry about that, I had run them right before a fairly simple change and forgot to run them again. Fixed.

On the other stuff, feel free to use this code or not... I needed it for a project I am doing and have put about as much time in as I can. Thanks!

@zerok
Copy link
Owner

zerok commented Jan 11, 2012

I'm now using django-hvad/nani in one of my projects and it seems like a nice solution to a really annoying problem. Perhaps there is a nice and easy way to get this integrated :-)

@nskeip
Copy link

nskeip commented Jun 14, 2012

Hi!
In flatblocks/template_tags/flatblock_tags.py we got the following code:

if 'django.middleware.locale.LocaleMiddleware' not in settings.MIDDLEWARE_CLASSES:
    logger.warning("For i18n support in flatblocks you must have django.middleware.locale.LocaleMiddleware in your MIDDLEWARE_CLASSES")
else:
    lang = get_language()

Well, it is rather nice to recieve a warnging like that, BUT for me (and other people who have a custom middleware instead of django.middleware.locale.LocaleMiddleware) it is a problem - I use Django CMS and it requires me not to use it and gives me some other middleware.

I suggest to just use get_language to get language :) and not to obligate user to use LocaleMiddleware:

if 'django.middleware.locale.LocaleMiddleware' not in settings.MIDDLEWARE_CLASSES:
    logger.warning("For i18n support in flatblocks you must have django.middleware.locale.LocaleMiddleware in your MIDDLEWARE_CLASSES")
    # yep, we still recieve a warning!)

lang = get_language()

tangochin pushed a commit to tangochin/django-flatblocks that referenced this pull request Sep 15, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants