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.

Sign up for GitHub

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

Jump to bottom

User playlists as grid #4949

Merged
merged 28 commits into from May 22, 2024
Merged

User playlists as grid #4949

merged 28 commits into from May 22, 2024

Conversation

jasonhenriquez
Copy link
Collaborator

@jasonhenriquez jasonhenriquez commented Apr 14, 2024 •
edited

User playlists as grid

Pull Request Type

  • Feature Implementation

Related issue

closes #4708

Description

  • Implements grid display option for user playlists
  • Attaches this behavior to the existing grid versus list Video View Type setting
  • Remote playlists will still use the list view, as has been the case
  • List view is still forced on mobile devices (<= 680px width threshold)

Screenshots

Note: picture is now outdated (missing Add to Playlist & Remove from Playlist icons, as well as "Sort by" added in other PR)

Screenshot_20240414_084944

Testing

  • Check editing the name and description of the user playlist to varying lengths
  • Check on mobile, tablet, & desktop views
  • Check remote playlists are not affected
  • Check Custom sort order left/right arrows and "remove from playlist" icon button functionality in grid view
  • Check no z-index tomfoolery with User Playlist grid view fixed top bar or ft-refresh-widget
  • (REG) Check user playlists with list type as "List"

Desktop

  • OS: OpenSUSE
  • OS Version: TW

Additional context

Implements the "fixed top bar" as a mixin that we can reuse if we want to make any other top sections fixed.

@github-actions GitHub Actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions GitHub Actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions GitHub Actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

Waiting on #4921 and #4929 to get merged first.

They're both merged :)

@github-actions GitHub Actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions GitHub Actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions GitHub Actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions GitHub Actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions GitHub Actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions GitHub Actions
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions GitHub Actions
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

Didnt look at the code but just putting this here as a friendly reminder to include in the PR

#5120 (comment)

@jasonhenriquez
Copy link
Collaborator Author

Yes, that's included in the merge commit

absidue
absidue reviewed May 21, 2024
View reviewed changes
Copy link
Member

@absidue absidue left a comment

Choose a reason for hiding this comment

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

Like it a lot better now, thank you. Just one small thing, we probably don't need the PLAYLIST_HEIGHT_FORCE_LIST_THRESHOLD stuff anymore now that the bar shrinks when you scroll.

@jasonhenriquez
Copy link
Collaborator Author

jasonhenriquez commented May 21, 2024 •
edited

I kept that because it's at a height where only one row of content is displayed. Any smaller, and the top bar occludes the main content too much (in the edge case of someone with a wide but short window).

@absidue
Copy link
Member

absidue commented May 21, 2024

In that case we can leave it in, if it were to caus issues for actual mobile users, it's not a hard thing to remove later on. I just thought I would mention it as it was added in response to one of my comments but didn't actually solve the issue (that was solved by collapsing the pinned content on scroll) and didn't seem particularly useful.

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: waiting for review For PRs that are complete, tested, and ready for review and removed PR: changes requested labels May 21, 2024
@jasonhenriquez
Copy link
Collaborator Author

That's fair. I kept it in a lower breakpoint than the original because I noticed the main content was inaccessible at a small enough height, but I never actually stated that explicitly, so I think that's fair to call out for sure.

efb4f5ff-1298-471a-8973-3d47447115dc
efb4f5ff-1298-471a-8973-3d47447115dc approved these changes May 21, 2024
View reviewed changes
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

@PikachuEXE
Copy link
Collaborator

Not blocking

  1. I found it weird that when I scroll down I see playlist action buttons (which I think not quite useful when scrolled down unlike search bar) but they can stay there is 2nd point below is resolved
  2. I expect to see the playlist title (not description or other stuff)

If it's too difficult to implement then it's fine to have this merged first

image

@jasonhenriquez
Copy link
Collaborator Author

I think it's doable, but the reason I prefer showing the action buttons is because you know what playlist you're looking at when you first land on it and most likely don't need a reminder, but you could gain from being able to do actions from any scroll level without having to scroll back up.

@PikachuEXE
Copy link
Collaborator

With multi-window I found it not quite the case for you know what playlist you're looking at (only works on first view)
Unless the window title shows part of the playlist name (which it doesn't right now) I would be a bit lost
As for action buttons I am fine for them to remain visible

@jasonhenriquez
Copy link
Collaborator Author

jasonhenriquez commented May 22, 2024 •
edited

Updating the window title to match the current playlist sounds like a good idea. I can do that in the same PR or a separate one, whichever you prefer.

Edit: 4154b64

@jasonhenriquez jasonhenriquez mentioned this pull request May 22, 2024
Merged
1 task
@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

Lets not push this back any further. Jason already created a separate PR so lets merge this :)

PikachuEXE
PikachuEXE approved these changes May 22, 2024
View reviewed changes
Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

Potential merge conflict incoming~

@FreeTubeBot FreeTubeBot merged commit 8a82abe into FreeTubeApp:development May 22, 2024
5 checks passed
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label May 22, 2024
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request May 23, 2024
@PikachuEXE
Merge branch 'development' into feature/video-added-to-playlist-count
fa578b6 
* development:
  Update playlist name with title (FreeTubeApp#5150)
  User playlists as grid (FreeTubeApp#4949)
  Add custom webpack loader to remove unused mimetypes from mime-db (FreeTubeApp#5148)
  ^ Update GH action eps1lon/actions-label-merge-conflict (FreeTubeApp#5034)
  Translated using Weblate (Italian)
  Translated using Weblate (Serbian)
  Translated using Weblate (Estonian)
  Translated using Weblate (Bulgarian)
  Translated using Weblate (Spanish)
  Translated using Weblate (Italian)
  Translated using Weblate (Polish)
  Translated using Weblate (Portuguese (Brazil))
  Translated using Weblate (French)
  Translated using Weblate (Chinese (Traditional))
  Translated using Weblate (Chinese (Simplified))
  Fix gap next to banner when Hide Side Bar Labels is enabled (FreeTubeApp#5120)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PikachuEXE PikachuEXE PikachuEXE approved these changes

@absidue absidue absidue approved these changes

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc approved these changes

@MarmadileManteater MarmadileManteater Awaiting requested review from MarmadileManteater

@ChunkyProgrammer ChunkyProgrammer Awaiting requested review from ChunkyProgrammer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Option to use grid display in playlists
7 participants
@jasonhenriquez @efb4f5ff-1298-471a-8973-3d47447115dc @MarmadileManteater @PikachuEXE @absidue @ChunkyProgrammer @FreeTubeBot