-
-
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
Add year selector to "Already Read" reading log page #8639
Add year selector to "Already Read" reading log page #8639
Conversation
…ing the new header. Added the 'year_dict' property to mybooks, retrieved from the ReadingLog class's new property, for populating the header.
…ept options once more.
… mybooks navigation.
for more information, see https://pre-commit.ci
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.
Thanks for updating this, @benbdeitch.
Due to errors, I wasn't able to run this locally. Errors are related to the new get_year_dict
function:
Aside from this, I have some suggestions:
- We want to populate the selector with years that the patron has set a yearly reading goal. Use
YearlyReadingGoals.select_by_username()
to find each year that a patron has set a goal. If you want, you can add anorder
kwarg, which defaults toyear ASC
. This way, you can passorder='year DESC'
to get the years in your desired order. - Store the reading goals in the MyBooksTemplate object as a property. It can be called something like
reading_goals
. Initialize this as an empty list. Also, create aselected_year
property and initialize it asNone
. - In
mybooks_readinglog.render_template()
, if thekey
isalready-read
andis_my_page
is true, fetch the reading goals and update the newMyBooksTemplate
property with the results. - Instead of passing the selected year to
/account/view.html
, updateMyBooksTemplate.selected_year
inreadinglog_yearly.GET()
. - If the
MyBooksTemplate
object has any reading goals, show the year selector. Options in the selector should not include counts --- only the year (or "All Time", localized).
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.
Thanks @benbdeitch!
Just a few more changes to go. In addition to the other suggested changes, I'm seeing that the "Share" link is pushed out of view on mobile screens:
You can fix this by adding the following rules to mybooks-details.less
:
.navigation-breadcrumbs {
display: flex;
flex-direction: row;
.crumb {
margin-right: 10px;
}
}
Within the media query:
.navigation-breadcrumbs {
flex-direction: column;
.crumb {
margin-bottom: 10px;
}
}
These changes should result in a mobile view that looks like this:
…eitch/openlibraryfork into #Redo-of-8572-Already-Read-Dropper
for more information, see https://pre-commit.ci
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8639 +/- ##
==========================================
- Coverage 16.68% 16.66% -0.02%
==========================================
Files 88 88
Lines 4681 4686 +5
Branches 835 835
==========================================
Hits 781 781
- Misses 3384 3389 +5
Partials 516 516 ☔ View full report in Codecov by Sentry. |
The requested changes have been made. Thanks for the assistance; I'll be keeping them in mind for the future. Out of curiosity, is there a specific reason behind ending files with a newline character? |
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.
|
for more information, see https://pre-commit.ci
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.
The style changes that I suggested led to unwanted results in tablet views:
Moving the flex direction changes to a mobile media query helped, but the selectors were not vertically aligned in tablet views:
Adding an align-items
rule to the tablet media query fixed things well enough (the share link's vertical alignment is a bit off in tablet views).
Closes #8557
This PR adds the ability to navigate to specific subsets of your already-read shelf, arranged by the year in which they were marked as read. If no books are available, the relevant dropper will not appear.
Technical
I reverted the breadcrumb_dropper in templates/books to a more generic form, instead adding two other files that fill it out with specific sets of values.
Testing
Add books to different years of your 'already-read' shelf, and the dropper will appear. The pages that it navigates to already exist, though they were only accessible by URL manipulation before now.
Stakeholders
@jimchamp