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

Page Range #2638

Merged
merged 5 commits into from
Feb 27, 2023
Merged

Page Range #2638

merged 5 commits into from
Feb 27, 2023

Conversation

Giebisch
Copy link
Contributor

@Giebisch Giebisch commented Jan 30, 2023

Implements #2609

Input for page range:

page_range_1

List quotes:

page_range_quotes

Still missing:

  • Tests

@Giebisch Giebisch changed the title Added Backend Part Page Range Jan 30, 2023
@Giebisch Giebisch marked this pull request as ready for review February 6, 2023 13:19
@hughrun
Copy link
Contributor

hughrun commented Feb 8, 2023

I can take a look at this in a few days, but before I do - @mouse-reeve any thoughts about this in relation to #2640? What do we want to do?

@mouse-reeve
Copy link
Member

Ah, I hadn't connected these two in my head! Inconclusive thoughts:

  • Having a formal page range meets a common use cases and would make the display consistent
  • Free text fields could contain page ranges, which might cause display weirdness
  • Having free text and page range would also allow things like "VI - VIII", which would be good to support

I guess having written that out I'm inclined to think that we should have a start and end, and make them both be free text fields?

@Giebisch
Copy link
Contributor Author

I think the current implementation probably covers the vast majority of cases. But we could perhaps create a new issue to request a third option for free texts besides "page" and "percent".

Copy link
Contributor

@hughrun hughrun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good, and I was scope-creeping earlier so I agree let's pull this in and deal with whether they should be int or a string separately.

See my comment: it would be good to ignore the second page number if it's the same as the first page.

The other thing I noticed is that if the user does not enter an initial page, the last page is ignored and it just posts without page numbers rather than throwing an error. I think that might be a better UX to be honest, but if you'd prefer to indicate to the user that they've made a mistake, or alternatively to treat the endposition in that case as a single page (i.e. treat it as the position) that will need to be dealt with.

bookwyrm/templates/snippets/status/content_status.html Outdated Show resolved Hide resolved
@Giebisch
Copy link
Contributor Author

Yep, that sounds like a good addition. From now on, the page will only be displayed once if start and end position are the same. I took the suggestion with the design, so the input should be more understandable for the user. So I think we don't need any further distinction of the cases here.

@Giebisch Giebisch requested a review from hughrun February 23, 2023 18:00
Copy link
Contributor

@hughrun hughrun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 😃

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