Skip to content
This repository has been archived by the owner on Sep 5, 2019. It is now read-only.

Archive search #159

Open
wants to merge 92 commits into
base: master
Choose a base branch
from
Open

Archive search #159

wants to merge 92 commits into from

Conversation

gh-PonyM
Copy link
Contributor

Adds the archive search page with tabs and pagination.
One issue I could not resolve were translations strings with variables:

  • ${x} items found
  • or Search in ${x}

I also added other translation strings like From date that are not ignored.
Is it reproducible?

Lukas added 30 commits August 5, 2019 12:54
- using .archive class more generally
- Does not conflict with endpoints existing
- entended_date_converter returns datetime.date in path.py
@gh-PonyM gh-PonyM requested a review from href August 13, 2019 07:19
@href
Copy link
Contributor

href commented Aug 13, 2019

If this is about snippets like this, you need another approach:

<strong tal:condition="archive_items" i18n:translate="">
Found ${item_count} items.
</strong>

To use variables in Chameleon templates, you need do something like this:

<strong tal:condition="archive_items" i18n:translate>
Found <tal:b i18n:name="item_count>${item_count}</tal:b> items.
</strong>

See https://chameleon.readthedocs.io/en/latest/reference.html#i18n-name

This is basically the same as providing the mapping argument in Python code.

Do you want to fix this before I do my review?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 94.928% when pulling 4a554ee on archive_search into d87b326 on master.

Copy link
Contributor

@href href left a comment

Choose a reason for hiding this comment

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

Looks good to me. There are some small issues, but nothing serious. Good job!

to_date=None,
types=None,
item_type=None,
domain=None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Since domain and answer are lists (i.e. contain a plural of elements), I'd name them domains and answers.

]

items = dict(
votes=[v for v in items if v.type == 'vote'],
Copy link
Contributor

Choose a reason for hiding this comment

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

If you don't intend to add any items to votes or elections, I would not use a list, instead use a tuple:

votes = tuple(v for v in items if v.type == 'vote')

Note that this is not the same as writing

votes = (v for v in items if v.type == 'vote')

Which would result in a generator, which is not what you want here. By using a tuple you use less memory and you communicate your intent of this list not changing later.

onegov/election_day/layouts/archive.py Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants