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

New feature: Show Subtitles #3117

Merged
merged 7 commits into from
Jul 9, 2024
Merged

New feature: Show Subtitles #3117

merged 7 commits into from
Jul 9, 2024

Conversation

mikiher
Copy link
Contributor

@mikiher mikiher commented Jul 4, 2024

Fixes #2774.

This re-introduces a feature removed from #3037 after some comments from @advplyr :

I don't think we should include the show subtitles in this PR.
It doesn't make sense to show that setting on the regular bookshelf view. It is too tight on mobile for the library page. I think we should use a context menu dropdown if we add more checkboxes to the bookshelf toolbar. Showing series and genres there confused me at first and when it is empty it looks weird.

This is a re-write of that feature taking those comments into account.

  • The Show/Hide Subtitles setting has been moved to the Toolbar's context menu dropdown
  • The subtitle field shows the subtitle from the metadata, if it exists, or the book's series and sequence if it doesn't.
    • No genres in this version, but I think series and sequence make sense if there's no subtitle.
    • This is only shown for books, not for podcasts
  • For a collpased-series entry, the subtitle shows the number of books in the series.

I think it also makes sense to move the Collapse Series option to the same menu. Would you like me to implement that?

@mikiher mikiher marked this pull request as ready for review July 4, 2024 17:58
@mikiher mikiher changed the title New feature: Show subtitles New feature: Show Subtitles Jul 4, 2024
@nichwall
Copy link
Contributor

nichwall commented Jul 4, 2024

I like the dropdown menu to show/hide subtitles, and I think moving Collapse Series option to the same menu is a good idea.

Some comments:

If collapse series and show subtitles are both enabled, should the icon in the upper right of the cover be removed?
collapse series

Slightly related, extra spacing is not added when sorting by "Publish Year". Extra spacing is added for all of the other sort methods.
Publish year sort
Modified sort

@nichwall
Copy link
Contributor

nichwall commented Jul 4, 2024

In regards to the series count in the upper right being removed, maybe that should be it's own option? There have been some requests in the past to allow it to be removed, so maybe this dropdown is the best place to add that.

@advplyr
Copy link
Owner

advplyr commented Jul 4, 2024

I was testing the card height issue and noticed that the shelf height isn't calculated correctly when sorting by title and having ignore prefixes when sorting enabled.

Highlighting the shelf div in inspector
image

@mikiher
Copy link
Contributor Author

mikiher commented Jul 4, 2024

Ah, thanks, I'll look into shelf height calculation bug.

@advplyr
Copy link
Owner

advplyr commented Jul 4, 2024

I just figured out that bug and will push the fix

@advplyr
Copy link
Owner

advplyr commented Jul 4, 2024

b01ef1c

@mikiher
Copy link
Contributor Author

mikiher commented Jul 4, 2024

Regarding @nichwall's comment:

If collapse series and show subtitles are both enabled, should the icon in the upper right of the cover be removed?

I don't think the icon and the subtitle text are mutually exclusive. I have usually have Collapse Series on, and I sometimes have a hard time seeing the icon (because of the book's cover color or just because my eye doesn't catch it). The text is just another signal that it's a slightly different thing.

@mikiher
Copy link
Contributor Author

mikiher commented Jul 4, 2024

b01ef1c

Good, nice catch!

@mikiher
Copy link
Contributor Author

mikiher commented Jul 4, 2024

I like the dropdown menu to show/hide subtitles, and I think moving Collapse Series option to the same menu is a good idea.

OK, moved the Collapse/Expand Series to the context menu.

Note: I did not put it in the seriesContextMenu, since Collapse Series doesn't seem to have any effect in the single series page anyway (rightly so?).

@mikiher
Copy link
Contributor Author

mikiher commented Jul 4, 2024

One other thing I noticed - check boxes in ABS seem to be mostly localized, but menu items are not. Is that on purpose?

@advplyr
Copy link
Owner

advplyr commented Jul 5, 2024

One other thing I noticed - check boxes in ABS seem to be mostly localized, but menu items are not. Is that on purpose?

I see 2 menu items that aren't localized in BookShelfToolbar.vue, are you referring to those or are there other menu items you are seeing?

@mikiher
Copy link
Contributor Author

mikiher commented Jul 5, 2024

One other thing I noticed - check boxes in ABS seem to be mostly localized, but menu items are not. Is that on purpose?

I see 2 menu items that aren't localized in BookShelfToolbar.vue, are you referring to those or are there other menu items you are seeing?

Sorry, my bad - I generalized from seeing the ones I saw in BookShelfToolbar - I now see most are localized.
There's just one other partially localized one in Appbar.vue.

I'll fix the un-loclalized menu items.

@mikiher
Copy link
Contributor Author

mikiher commented Jul 5, 2024

Done.

@advplyr
Copy link
Owner

advplyr commented Jul 9, 2024

Looks good, thanks!

@advplyr advplyr merged commit 10cb8eb into advplyr:master Jul 9, 2024
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.

[Enhancement]: Toggle to display subtitle in library
3 participants