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

Series page: Add pagination #52

Merged
merged 6 commits into from
Feb 20, 2017
Merged

Series page: Add pagination #52

merged 6 commits into from
Feb 20, 2017

Conversation

torsava
Copy link
Contributor

@torsava torsava commented Feb 5, 2017

Closes issue #8.

I included a refactoring commit to make the subsequent commit's diff easy on the eyes.

@torsava
Copy link
Contributor Author

torsava commented Feb 6, 2017

The checks fail saying some URL is unreachable, though I haven't been yet able to find out why exactly. I'll try to do so soon.

@encukou
Copy link
Member

encukou commented Feb 6, 2017

Your code assumes there's a meetup in the current year. The test data doesn't include such a meetup.

pyvocz/views.py Outdated
organizer_info=organizer_info)
return render_template('series.html', series=series, today=today, year=year,
organizer_info=organizer_info, first_year=first_year,
last_year=last_year, series_slug=series_slug)
Copy link
Member

Choose a reason for hiding this comment

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

Passing series_slug is unnsecessary; you can use series.slug.

year=last_year if year==None else year-1) }}">&gt;</a>
</li>
</ul>
</div>
Copy link
Member

Choose a reason for hiding this comment

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

Please use a local macro instead. In series.html, you can write:

{% macro pagination() %}
<div>
    ...
</div>
{% endmacro %}

and then use it as {{ pagination() }}.

The "fragments" are an embarassing artefact from a time when I didn't know about macros. (I wrote commits to get rid of them, but didn't submit them yet as I don't want to conflict with this work.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do!

pyvocz/views.py Outdated
@route('/<series_slug>/')
def series(series_slug):
@route('/<series_slug>/', defaults={'year': None})
@route('/<series_slug>/<int:year>/')
Copy link
Member

Choose a reason for hiding this comment

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

I think it would also be useful to have a page that shows all the meetups.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I absolutely forgot about that when I was finishing it on the weekend. Will do!

@torsava
Copy link
Contributor Author

torsava commented Feb 7, 2017

I believe I've addressed the code modifications, though I still haven't made progress on the test failure.

@torsava
Copy link
Contributor Author

torsava commented Feb 7, 2017

Is there a way to run the pyvocz server with the test data so I can see what it's actually doing?

@encukou
Copy link
Member

encukou commented Feb 8, 2017

To reproduce this issue, it should be enough to clone https://github.com/pyvec/pyvo-data and remove the meetups for 2017 and 2016 from one of the series. Then pass in the directory with --data.

The files used for tests are listed in conftest.py.

@torsava
Copy link
Contributor Author

torsava commented Feb 19, 2017

So, I think all issues are addressed, tests are passing, and for good measure I made it so years with no events are not displayed as options in the pagination!

@encukou encukou merged commit edc07f2 into pyvec:master Feb 20, 2017
@encukou
Copy link
Member

encukou commented Feb 20, 2017

Looks pretty good to me, thanks!

Deployed.

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