-
Notifications
You must be signed in to change notification settings - Fork 40
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
Fix plurality of mementos and resources on main replay page. Closes #534 #535
Conversation
Codecov Report
@@ Coverage Diff @@
## master #535 +/- ##
=========================================
- Coverage 24.41% 23.5% -0.92%
=========================================
Files 6 6
Lines 1151 1353 +202
Branches 170 260 +90
=========================================
+ Hits 281 318 +37
- Misses 853 1010 +157
- Partials 17 25 +8
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than manually pluralizing and passing too many variables, I would encourage using inflect
library and making the function available as a filter in the template by utilizing Context Processors.
@ibnesayeed That would be a good improvement but for now, this fixes the issue described in #534. |
|
I will leave the decision of merging this PR in its current state or otherwise to you discretion because I am not a big fan of hacky approaches on every step that pile up and make maintenance difficult (and invite more hacks for every new change). |
We can avoid using that, but still move the pluralization logic into Context Processors. |
@ibnesayeed Ok, I'll get a little more familiar with Flask's context processors and implement that with this same PR. |
@ibnesayeed I updated this to use a context processor. Please have another look. |
|
@ibnesayeed Kudos on the implementing the templating system, btw!