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

Modify site template to fix issue #169 for meetup display #174

Merged
merged 1 commit into from
May 9, 2015

Conversation

willingc
Copy link
Member

@willingc willingc commented May 9, 2015

Fixes the meetup display on home page.

Add an additional check to see if the meetup_id exists for a site. If so, add the meetup to the list of meetup_ids.

@willingc
Copy link
Member Author

willingc commented May 9, 2015

@audreyr @econchick If either of you can review this fix and merge, it should correct meetup display on the homepage. Not sure if there should be a trailing ; at the end of the changed statement. It works without, but js is not my thing ;) If it should have one, I'm happy to edit to update the PR.

@audreyfeldroy
Copy link
Member

Thank you @willingc!

I'd say yes on adding a trailing ;. Generally they're good to put in with standard JS projects. There are a few weird exceptions of projects that don't use the trailing semicolons, but they're the odd ones out.

Another thing I suggest is breaking the line into separate lines, to make it clearer which if goes with which endif. To do this without adding extra whitespace, you can add a minus sign to the blocks, as shown in http://jinja.pocoo.org/docs/dev/templates/#whitespace-control.

Let us know if you need help :)

@willingc
Copy link
Member Author

willingc commented May 9, 2015

Thanks @audreyr :)
So I broke up the line using the whitespace-control (adding minus signs to the blocks). It runs locally. It looks a little ugly to me, like there should be an easier or cleaner way. If there is, please let me know. Thanks.

@audreyfeldroy
Copy link
Member

Awesome, thanks so much @willingc.

It looks as good as it can, don't worry. With Jinja-templated JS, you can only get so clean, and what you've done is great.

audreyfeldroy added a commit that referenced this pull request May 9, 2015
Modify site template to fix issue #169 for meetup display
@audreyfeldroy audreyfeldroy merged commit 26dfa52 into pyladies:master May 9, 2015
@audreyfeldroy
Copy link
Member

I just verified that it works locally, and that the JS renders without breaking lines. @econchick, this is good to go, just pull and deploy :)

@willingc
Copy link
Member Author

willingc commented May 9, 2015

@audreyr Thanks for the tips and the review. I learned some JS today. I also learned it's not as pretty as Python :)

@econchick
Copy link
Member

Deployed - thank you @willingc and @audreyr !

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.

None yet

3 participants