-
Notifications
You must be signed in to change notification settings - Fork 1
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
Two proposals in this PR: Python 3.4 support and Exposing current tab instance in template #12
Conversation
Instead of passing specific Tab properties, it's probably much better to pass the whole object. Keeping old variables for backwards compatibility.
I like the changes, thanks for contributing! Please do the following things before I merge this:
|
@@ -143,6 +143,8 @@ def get_context_data(self, **kwargs): | |||
'group_current_tab': self, | |||
} | |||
context['tabs'] = self._process_tabs(**process_tabs_kwargs) | |||
# Expose the instance itself to ease access to any other property | |||
context['current_tab'] = self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parent class of this view is TemplateView
which uses the ContextMixin
. This adds an instance of the view to context as variable view
. So this is simply a duplication of the code inside ContextMixin
. If this would be documented there wouldn't be the need to expose the view under a different name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, funny that you commented just now. That fact also occured to me 5 minutes ago. So the current_tab
variable is entirely unnecessary.
I'll add documentation for the view
variable instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right. Sorry for my mistake. Can you merge just the first
commit for Python 3 support? I will modify the PR if necessary on Monday.
If you want I can document this as suggested as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, will do.
Python 3 commit merged in 355dfaa. Documentation improved in a308415. Version 0.4.0 is out! https://pypi.python.org/pypi/django-tabination |
Both commits are independent (even though consecutive and you can merge both if your like them), both cover testing and documentation independently. Hope you accept them :)
In any case, thanks for your nice project.