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

Fixed layout issue to prevent needing to replace WidgyPage #257

Closed
wants to merge 3 commits into from

Conversation

zmetcalf
Copy link
Contributor

@gavinwahl, I took what you gave on the google groups and added in a little bit with layout. What do you think?

I have a project I tried it out on here: https://github.com/zmetcalf/layout_issue

@property
def owner_class(self):
if 'widgy.contrib.widgy_mezzanine' in settings.INSTALLED_APPS:
from widgy.contrib.widgy_mezzanine.models import WidgyPage
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gavinwahl, do I need to put a try/except here because of the potential of an ImportError?

@gavinwahl
Copy link
Member

I don't understand this owner_class thing

@zmetcalf
Copy link
Contributor Author

In the valid_root_of method you provided, owner_class in the function only produces the widgy.contrib.widgy_mezzanine.models.WidgyPage, so it needs something to compare it to. I added in an owner_class property so it could tell the difference between a WIDGY_MEZZANINE_PAGE_MODEL and lets say a Blog model. Does that help?

@gavinwahl
Copy link
Member

I was thinking it would look like this:

def valid_root_of(self, owner_class, root_class, root_choices):
    if isinstance(owner_class, WidgyPage):
        return isinstance(root_class, (YourLayout,))
    else:
        return super().valid_root_of(owner_class, root_class, root_choices)

Unrelated, I'm wonder why valid_root_of returns a boolean instead of having a root_choices_for that returns a list of possible roots.

@zmetcalf
Copy link
Contributor Author

For the returning of booleans, I would say it is because the Generators function that calls it requires a boolean, not the actual list. I am not sure what you meant by root_choices_for

I will try putting the necessary logic in valid_root_of.

@zmetcalf
Copy link
Contributor Author

@gavinwahl, I think I see where you are going with this. I kept it with just what you put on google groups, then I overrided site.py in my test project to make it work. https://github.com/zmetcalf/layout_issue/blob/master/widgy_demo/site.py

What do you think of adding in a setting?


def valid_root_of(self, owner_class, root_class, root_choices):
   if settings.WIDGY_MEZZANINE_LAYOUT_LIST:
        return issubclass(root_class, settings.WIDGY_MEZZANINE_LAYOUT_TUPEL)
    else: 
        return issubclass(root_class, root_choices)

@zmetcalf
Copy link
Contributor Author

On your example, the second line will always return False. If it is changed to issubclass, it will always be true if there is no WIDGY_MEZZANINE_PAGE_MODEL declared, so it won't weed out a BlogLayout because owner_class just returns the active page model.

@zmetcalf
Copy link
Contributor Author

@gavinwahl, sorry for being a mess. I updated the test project so it works now: https://github.com/zmetcalf/layout_issue/blob/master/widgy_demo/site.py

@rockymeza
Copy link
Contributor

I think @gavinwahl means doing something like this:

def get_root_choices_for(self, owner_class):
    if issubclass(owner_class, WidgyPage):
        return ('myapp.MyBaseLayout',)
    else:
        return owner_class.root_node.root_choices

HOWEVER, what I just realized is that we are dealing with the owner_class, which is actually breaking one of our eventual use-cases of allowing a model to have multiple WidgyFields. This method and the valid_root_of as is don't support multiple WidgyFields.

@rockymeza rockymeza closed this Sep 19, 2014
@rockymeza
Copy link
Contributor

I didn't mean to close it, sorry!

@rockymeza rockymeza reopened this Sep 19, 2014
@zmetcalf
Copy link
Contributor Author

No worries, I am going to close it anyway because it looks like you all need to figure out the future of owner_class. (and it is Gavin's code anyway)

@zmetcalf zmetcalf closed this Sep 19, 2014
julianandrews pushed a commit that referenced this pull request Mar 2, 2016
This should allow better control of which layouts are available for a
given site.

This is basically Gavin Wahl's change from
#257 but passing the field
instead of the model to address Rocky Meza's concern about multiple
`WidgyField`s on a single model.
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.

3 participants