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

Fallback to showing first published date #3003

Merged
merged 2 commits into from
Oct 17, 2023

Conversation

dato
Copy link
Contributor

@dato dato commented Sep 19, 2023

First publication date is only shown when the editor is not known
(rationale: if the editor for the first edition is known, the same
date should be set in both fields?).

@mouse-reeve
Copy link
Member

This is good, but I think there's a better way to handle this that's a slightly more complicated change. We can use {% with %} to set a variable to firstof the two available date options, and then proceed with all the boolean logic using one variable, which will be a heck of a lot simpler. Does that make sense?

@dato
Copy link
Contributor Author

dato commented Sep 23, 2023

Sure, done that in 2c968e9.

I initially didn't want to allow combining publisher with first_published_version, but this version allows it. If we want to disallow it, it's an easy extra fix (ec2c5cb). Let me know what you prefer.

@mouse-reeve
Copy link
Member

Ah I didn't even think of that but it would look extremely weird to say a book was released by a publisher before the publisher existed. I think your additional change is a good solution

@dato
Copy link
Contributor Author

dato commented Oct 2, 2023

Done.

As a plus, that part of the template didn't grow, but even shrank by (net) three lines:

❯ g diff --stat @^^
 bookwyrm/templates/book/publisher_info.html | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

Thanks!

@jaschaurbach jaschaurbach merged commit 06923c6 into bookwyrm-social:main Oct 17, 2023
10 checks passed
@dato dato deleted the book_info_first_pub_date branch October 20, 2023 02:10
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.

3 participants