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

#240 Page views and Page context classes #262

Merged
merged 6 commits into from
Feb 14, 2019
Merged

#240 Page views and Page context classes #262

merged 6 commits into from
Feb 14, 2019

Conversation

duker33
Copy link
Collaborator

@duker33 duker33 commented Feb 11, 2019

Closes #240

  • Create page views display. It's explicit class module with classes for DB templates processing
  • Create page context. Move base context classes to pages module

@duker33 duker33 self-assigned this Feb 11, 2019
self.page_view = page_view

def __getattr__(self, item):
if item in self.STORED:
Copy link
Contributor

Choose a reason for hiding this comment

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

let raise AttributeError if an item not in STORED

return self.page_view.render(item)


class Page:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
class Page:
class RenderedPage:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@artemiy312 , let's look at the shopelectro.views.catalog.CategoryPage class for example.
Here we have pretty the same situation: view uses some template and given context to render response body. But we don't call it RenderedCategoryPage.

So, let's leave the Page name

@@ -0,0 +1,40 @@
"""
Views processing db based templates.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we spawn new View entity? I believe that this will lead to ambiguity, because we will have two type of different things with the same name. I'd prefer to avoid this term.

We may use this as part of context classes instead of db view or just use these classes without mentions about View

Up to you

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@artemiy312 , this class has the view nature. That's why every other name for this class will be bad.
It's not good to spawn the one other view module, i agree.
The best approach is to put the class to the existing view module. But we'll have problems with cyclic imports in this case.
So, new module name is just trade off to make django's standard design pleasure.

I'll add some of this words to the top of db_view module and leave it as is

Copy link
Contributor

Choose a reason for hiding this comment

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

this class has the view nature

But it isn't. A view is a callable which takes a request and returns a response. These classes don't match with this term

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@artemiy312 , y, you are right. So, we need some term for "template renderer with context".
I propose "Display". We already used it as part of property name page.display_h1 for example.

I'll rename module

@duker33 duker33 merged commit f0a7334 into master Feb 14, 2019
@duker33 duker33 deleted the 240_page_context branch February 14, 2019 08:28
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.

2 participants