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

ui: add tag entries page #2527

Merged
merged 1 commit into from
Apr 15, 2024
Merged

ui: add tag entries page #2527

merged 1 commit into from
Apr 15, 2024

Conversation

rdelaage
Copy link
Contributor

fix #2522

This PR bring a page to show entries related to a same tag

I open this draft PR to get some suggestions on that feature

  • I am not sure about the type used to store tags in the database currently (text[]). This is not the more optimal, a dedicated table would be better I think

Do you follow the guidelines?

@rdelaage rdelaage force-pushed the feat/tag_page branch 3 times, most recently from 975b33c to de6e842 Compare March 22, 2024 18:21
@rdelaage rdelaage mentioned this pull request Mar 23, 2024
4 tasks
@rdelaage
Copy link
Contributor Author

A preview of the feature running on a production instance
image
image
image

@rdelaage rdelaage force-pushed the feat/tag_page branch 6 times, most recently from e88282a to 51ee18d Compare March 28, 2024 19:31
@rdelaage
Copy link
Contributor Author

Performance seems to be ok with the text[] structure in database. I will mark this PR as ready, feel free to make suggestions!

@rdelaage rdelaage marked this pull request as ready for review March 28, 2024 19:34
@rdelaage rdelaage changed the title WIP: tag entries page ui: add tag entries page Mar 28, 2024
@rdelaage rdelaage force-pushed the feat/tag_page branch 4 times, most recently from 01fe46e to b92f5c1 Compare April 2, 2024 11:06
@rdelaage
Copy link
Contributor Author

rdelaage commented Apr 3, 2024

@fguillot Can I have a feedback on this feature and a review of the PR?

Copy link
Member

@fguillot fguillot left a comment

Choose a reason for hiding this comment

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

I'm not sure if separating the unread and all article views on the tags page makes sense in this context.

Below is an example in a screen recording where I open a read entry and click on a tag. The fact that the page doesn't display any entries might confuse people.

Recording.2024-04-03.203743.mp4

internal/template/functions.go Outdated Show resolved Hide resolved
internal/template/templates/views/tag_entries.html Outdated Show resolved Hide resolved
@rdelaage
Copy link
Contributor Author

rdelaage commented Apr 4, 2024

I'm not sure if separating the unread and all article views on the tags page makes sense in this context.

Below is an example in a screen recording where I open a read entry and click on a tag. The fact that the page doesn't display any entries might confuse people.
Recording.2024-04-03.203743.mp4

Not sure about what to do. Both unread and all entries pages are interesting in my use case. Maybe I can order entries to show unread entries first (order by status and date instead of only date)

@rdelaage rdelaage force-pushed the feat/tag_page branch 2 times, most recently from de7bfae to 1f26aba Compare April 4, 2024 18:34
@rdelaage rdelaage requested a review from fguillot April 4, 2024 18:35
@rdelaage rdelaage force-pushed the feat/tag_page branch 3 times, most recently from 6104ce9 to 5067cd1 Compare April 10, 2024 12:05
@rdelaage
Copy link
Contributor Author

@fguillot Sorry to ping you again but can you review my PR again please?

I can order entries to show unread entries first (order by status and date instead of only date)

I implemented this instead of having both unread and all pages and I would like to know your opinion on this :)

@fguillot
Copy link
Member

@fguillot Sorry to ping you again but can you review my PR again please?

I can order entries to show unread entries first (order by status and date instead of only date)

I implemented this instead of having both unread and all pages and I would like to know your opinion on this :)

Sounds good.

@fguillot fguillot merged commit 647c66e into miniflux:main Apr 15, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Tag entries page
2 participants