-
Notifications
You must be signed in to change notification settings - Fork 735
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
limit feed/category entry pagination to unread when coming from unread entry list #2138
Conversation
internal/ui/entry_category.go
Outdated
@@ -53,6 +53,19 @@ func (h *handler) showCategoryEntryPage(w http.ResponseWriter, r *http.Request) | |||
|
|||
entryPaginationBuilder := storage.NewEntryPaginationBuilder(h.store, user.ID, entry.ID, user.EntryOrder, user.EntryDirection) | |||
entryPaginationBuilder.WithCategoryID(categoryID) | |||
|
|||
// Limit pagination to unread unless 'all' query param is specified | |||
if !request.HasQueryParam(r, "all") { |
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.
Actually I think
if request.HasQueryParam(r, "unread")
is easier to read.
Reduce the usage of "Not" "!" .
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.
Also not all
could mean only unread
. It could also mean only read
(in case anyone wants it).
So be more specific only unread
sounds better to me.
@@ -88,7 +88,7 @@ <h3>{{ t "alert.feed_error" }}</h3> | |||
{{ if ne .Feed.Icon.IconID 0 }} | |||
<img src="{{ route "icon" "iconID" .Feed.Icon.IconID }}" width="16" height="16" loading="lazy" alt="{{ .Feed.Title }}"> | |||
{{ end }} | |||
<a href="{{ route "feedEntry" "feedID" .Feed.ID "entryID" .ID }}">{{ .Title }}</a> | |||
<a href="{{ route "feedEntry" "feedID" .Feed.ID "entryID" .ID }}{{ if not $.showOnlyUnreadEntries }}?all{{ end }}">{{ .Title }}</a> |
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 don't think there is an agreement on
whether
/category/<category_id>/entry/<entry_id>?unread=1
or
/category/<category_id>/unread/entry/<entry_id>
/category/<category_id>/entry/<entry_id>?unread=1
Looks easier here
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.
Or 'unread/category/id/entries' ?
As we already have 'unread' route and we can keep 'category/id/entries' part consistent.
internal/ui/entry_category.go
Outdated
@@ -53,6 +53,19 @@ func (h *handler) showCategoryEntryPage(w http.ResponseWriter, r *http.Request) | |||
|
|||
entryPaginationBuilder := storage.NewEntryPaginationBuilder(h.store, user.ID, entry.ID, user.EntryOrder, user.EntryDirection) | |||
entryPaginationBuilder.WithCategoryID(categoryID) | |||
|
|||
// Limit pagination to unread unless 'all' query param is specified | |||
if !request.HasQueryParam(r, "all") { |
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.
Also not all
could mean only unread
. It could also mean only read
(in case anyone wants it).
So be more specific only unread
sounds better to me.
Also resolves #533 |
4110349
to
889651c
Compare
Updated to use /unread route instead of query parameters. |
Hoping this branch gets merged 😀. |
resolves #1212
Do you follow the guidelines?