-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 "Previews available in:" sometimes displayed without any preview languages #6408 #6410
Conversation
Made the caption "Previews available in:" appear only when they are available preview languages.
for more information, see https://pre-commit.ci
Previews available in: | ||
$for e in previews: | ||
$if len(e.languages)!=0: | ||
Previews available in: |
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.
Question, will this result in the words "Previews available in" being displayed multiple times in the page?
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.
I see! We have a second for loop and are using breaks
, that can work!
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.
By default, any non-zero number will be a True
value, so can use len(e.languages)
as an a equivalent to len(e.languages)!=0
.
Another option is... on line L269 we can turn this into a list comprehension where we do something like...
$if previews and any([len(e.languages) for e in previews)]:
<p class="preview-languages">
Previews available in:
$for e in previews:
$if e.languages[0].name not in seen:
$ seen.add(e.languages[0].name)
<a href="$work.key?edition=$(e.ocaid)">$e.languages[0].name</a>
</p>
This way we don't render <p class="preview-languages">
if its not going to be used
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.
Thank you! Do you want me to change it in the way that you suggested?
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.
I implemented the changes
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.
Lgtm, and seems to be working on testing:
https://testing.openlibrary.org/works/OL4472450W/Shad?edition=ia%3Ashad0000corr
Compare to production:
https://openlibrary.org/works/OL4472450W/Shad?edition=ia%3Ashad0000corr
Made the caption "Previews available in:" appear only when they are available preview languages.
Closes #6408
Screenshot
Stakeholders
@jimchamp