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

Post improvements #1224

Merged
merged 4 commits into from Oct 19, 2017
Merged

Post improvements #1224

merged 4 commits into from Oct 19, 2017

Conversation

noirbizarre
Copy link
Contributor

This PR brings some improvements to post display and listing:

  • allows to enable discussions on posts with POST_DISCUSSIONS_ENABLED
  • adds some basic post navigation (previous and next post, link to post list)
  • make the post list discoverable and style it
  • import some post page style that should be in core from udata-gouvfr

screenshot-www data dev-2017-10-19-10-56-16-361

screenshot-www data dev-2017-10-19-10-57-07-775

screenshot-www data dev-2017-10-19-10-57-36-610

I think this is only a first pass, navigation and search needs some styling, there might be some things missing but it's a start


**default** `False`

Wether or not discussions should be enabled on posts
Copy link
Contributor

@abulte abulte Oct 19, 2017

Choose a reason for hiding this comment

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

s/wether/whether/

Wether is a prime example of a word that will slip past the spell check. It is easily confused with two of its homonyms, whether and weather. Flying fingers find it easy to miss the single letter that separates them. Unless you’re a farmer, you might not even know that wether is either a:
male sheep or ram (the Oxford Dictionary of Etymology traces its roots to Old English, Old High German, Old Norse and Goth)
or a:
castrated ram or billy goat (according to A Word A Day).

😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😂

@@ -3,7 +3,9 @@

from udata import mail
from udata.i18n import lazy_gettext as _
from udata.models import Dataset, Reuse
from udata.core.dataset.models import Dataset
Copy link
Contributor

Choose a reason for hiding this comment

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

why the switch from udata.models? Just curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As much as possible and when I can think about it, I try to remove improt from the udata.models facade:

  • it's hard to refactor and introspect
  • it's one of the circular dependencies sources
  • it's one of the things that increase the test duration

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like something which could benefit of an issue scheduled for 1.3 or something :-)



@sitemap.register_generator
def sitemap_urls():
yield 'posts.list_redirect', {}, None, "weekly", 0.6
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the code below unreachable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. After this yield statement, I believe the interperter is processing the for loop.
What do you think prevent it ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I always thought of yield as a kind of return but after a quick test I realize I was wrong ;-)

try:
params_page = int(request.args.get('page', 1) or 1)
return max(params_page, 1)
except:
Copy link
Contributor

Choose a reason for hiding this comment

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

More precise exception catching possible?

params_page_size = request.args.get('page_size',
self.default_page_size)
return int(params_page_size or self.default_page_size)
except:
Copy link
Contributor

Choose a reason for hiding this comment

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

Idem.

@noirbizarre noirbizarre merged commit b947d39 into opendatateam:master Oct 19, 2017
@noirbizarre noirbizarre deleted the post-improvements branch October 19, 2017 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants