Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tinymce integration / template tags #2

Closed
wants to merge 3 commits into from
Closed

tinymce integration / template tags #2

wants to merge 3 commits into from

Conversation

littlepea
Copy link

This code resolves #1.

Also added simple template tags to access and filter artworks outside of portfolio views.

@dokterbob
Copy link
Owner

Hey Evgeny, thanks for your contribution!

However, there are two main issues with your pull request:

  1. Having a WYSIWYG-editor is (very) nice. However, tinymce is rather dated and the maintenance of django-tinymce is questionable (I've had so many obscure errors with it in production environments) that at the very least I'd like there to be a choice available. I think at least the much more modern imperavi should be supported. Look here for a good hint on how to implement editor-pluggability. As an added benefit this would make it possible to retain backwards-compatibility (upgrades don't force the use of the WYSIWYG-editor).
  2. The template tags are nice but they involve some significant design decisons. I think this should be a seperate patch. Morever, I don't personally like implicit inclusion tags - there is an excellent {% include %} tag available in Django and together with explicit assignment tags {% artworks collection="bananas" as banana_collection %} they provide a more elegant alternative. This way, the implementer can choose whether to use a seperate tag or not and it's allways crystal clear what context is available in what template. Lastly, I think template tags should have some sort of test coverage.

I hope you can agree with the aforementioned points. I realise I'm asking for a lot, though your contributions make a lot of sense: I would really like to include the features you implemented, albeit implemented in a different way. Moreover; your code is nice and clean and well documented so I'd like to see more of it. ;)

@littlepea
Copy link
Author

Hello, thanks for the explanation.

  1. I don't find tinymce to be the optimal editor neither. But I did find it the easiest to setup out-of-the-box. Some others take a long time to setup before they're actually functional, or require a lot of additional settings.
    What if I implement django-imperavi and make it optional (apply only if it's importable / or if it's enabled by a certain setting and importable)?
    I just don't feel like working on pluggable editors :)
    Or I can implement a setting like ARTWORK_EDITOR (None by default) with only one possible value except None being 'imperavi'. And if somebody wants to add another editor support they can do it by implementing it and adding more possible values for this setting?
  2. I can rework the tags to be as-tags, no problem. And will try to add tests.

Making separate patches is a bit annoying though because you can only create one fork for one repo. And if I create a pull-request it doesn't freeze at any specific commit, so when I commit some more after I've already created a pull-request it keeps adding new commits to the same request...
I couldn't find an answer for it in Github help. Do you know a proper workflow for it? Shall I create feature-branches after I fork? Is it even possible as fork is already a branch?

@littlepea littlepea closed this Mar 18, 2013
@dokterbob
Copy link
Owner

Hello, thanks for the explanation.
Thanks for the quick response!

I don't find tinymce to be the optimal editor neither. But I did
find it the easiest to setup out-of-the-box. Some others take a long
time to setup before they're actually functional, or require a lot
of additional settings.
What if I implement django-imperavi and make it optional (apply only
if it's importable / or if it's enabled by a certain setting and
importable)?
I just don't feel like working on pluggable editors :)
Or I can implement a setting like ARTWORK_EDITOR (None by default)
with only one possible value except None being 'imperavi'. And if
somebody wants to add another editor support they can do it by
implementing it and adding more possible values for this setting?

If you look at the patch I suggested, you'll find you can copy most of
the code from there. It is well-tested, as a bonus.

I can rework the tags to be as-tags, no problem. And will try to add
tests.

Thanks a lot!

Making separate patches is a bit annoying though because you can only
create one fork for one repo. And if I create a pull-request it doesn't
freeze at any specific commit, so when I commit some more after I've
already created a pull-request it keeps adding new commits to the same
request...
I couldn't find an answer for it in Github help. Do you know a proper
workflow for it? Shall I create feature-branches after I fork? Is it
even possible as fork is already a branch?
Yes, feature branches is the way to go here. It is easy firing a pull
req for branches and you can keep updating them if something's wrong
with a pull req. A branch is in essence the same as a fork (the
difference being mostly a URL).

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.

Use WYSIWYG editor for text fields
2 participants