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

Separate tabs for pending and downloaded in media history #508

Merged

Conversation

robertkleinschuster
Copy link
Contributor

What's new?

N/A

What's changed?

Media history separated tabs for downloaded and pending media items.

What's fixed?

N/A

Any other comments?

N/A

  • I am the original author of this code and I am giving it freely to the community and Pinchflat project maintainers
Bildschirmfoto 2024-12-11 um 21 32 12 1

Copy link
Owner

@kieraneglin kieraneglin left a comment

Choose a reason for hiding this comment

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

Thank you for this! I've requested a few small changes 🤙

Comment on lines 101 to 104
defp generate_base_query("pending") do
MediaQuery.new()
|> MediaQuery.require_assoc(:media_profile)
|> where(^dynamic(^MediaQuery.downloaded() or ^MediaQuery.pending()))
Copy link
Owner

Choose a reason for hiding this comment

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

Since this is just for pending media items, could you please remove the MediaQuery.downloaded() from this query?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah shoot, forgot to change it. First i was thinking about a "All" tab. Then i realized there are also the "other" entries.

Comment on lines 61 to 65
media_state = session["media_state"]
base_query = generate_base_query(media_state)
pagination_attrs = fetch_pagination_attributes(base_query, page)

{:ok, assign(socket, Map.merge(pagination_attrs, %{base_query: base_query}))}
{:ok, assign(socket, Map.merge(pagination_attrs, %{base_query: base_query, media_state: media_state}))}
Copy link
Owner

Choose a reason for hiding this comment

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

nit: you don't need to pass media_state in the new assigns since we're not currently doing anything with that outside of the mount function.

Since that's the case, I figure you can also save the intermediate variable and just use:

base_query = generate_base_query(session["media_state"])
# ...
{:ok, assign(socket, Map.merge(pagination_attrs, %{base_query: base_query}))}

@kieraneglin
Copy link
Owner

Looks great! Thank you

@kieraneglin kieraneglin merged commit e56f39a into kieraneglin:master Dec 13, 2024
1 check passed
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.

[FR] Implement a "Feed" of downloaded videos
2 participants