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

add playlist progress bar #2402

Merged
merged 8 commits into from
Aug 16, 2022
Merged

Conversation

makerio90
Copy link
Contributor

could use some css


add playlist progress bar

Important note
We may remove your pull request if you do not use this provided PR template correctly.

Pull Request Type
Please select what type of pull request this is:

  • Bugfix
  • Feature Implementation

Related issue
Please link the issue your pull request is referring to. If this pull request fully resolves the relevant issue, put "closes" before the issue number. Example: "closes #123456".

Description
when whatching a playlist, it displays a little progress bar to show you your progress through the playlist.

Screenshots (if appropriate)
Please add before and after screenshots if there is a visible change.
image

Testing (for code that is not small enough to be easily understandable)
Has this pull request been tested? y
Please describe shortly how you tested it and whether there are any ramifications remaining.
it works.
Desktop (please complete the following information):

  • OS: [manjaro]
  • OS Version: [n/a]
  • FreeTube version: [latest]

Additional context
Add any other context about the problem here.
needs css

could use some css
@PrestonN PrestonN enabled auto-merge (squash) July 16, 2022 21:18
@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added the PR: waiting for review For PRs that are complete, tested, and ready for review label Jul 18, 2022
Copy link
Member

Choose a reason for hiding this comment

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

fix lint warnings

Would also like to see the color of the progress bar gets adjusted to the color u choose in the settings under Theme settings > secondary theme settings

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Jul 18, 2022
idk if those changes to the package.json or yarn.lock are ok but
it wouldent lint without them :/
auto-merge was automatically disabled July 18, 2022 21:59

Head branch was pushed to by a user without write access

@PrestonN PrestonN enabled auto-merge (squash) July 18, 2022 21:59
@makerio90
Copy link
Contributor Author

fix lint warnings

Would also like to see the color of the progress bar gets adjusted to the color u choose in the settings under Theme settings > secondary theme settings

fixed and fixed, although the theming is a bit odd.

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

efb4f5ff-1298-471a-8973-3d47447115dc commented Jul 18, 2022

30k lines added and 7k removed? I think something didnt go well.

@makerio90
Copy link
Contributor Author

30k lines added and 7k removed? I think something didnt go well.

i dont think i was supposed to track yarn.lock or package-lock.json

auto-merge was automatically disabled July 18, 2022 23:38

Head branch was pushed to by a user without write access

@PrestonN PrestonN enabled auto-merge (squash) July 18, 2022 23:38
auto-merge was automatically disabled July 18, 2022 23:42

Head branch was pushed to by a user without write access

@PrestonN PrestonN enabled auto-merge (squash) July 18, 2022 23:42
auto-merge was automatically disabled July 18, 2022 23:45

Head branch was pushed to by a user without write access

@PrestonN PrestonN enabled auto-merge (squash) July 18, 2022 23:45
@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 Jul 19, 2022
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

@PrestonN
Copy link
Member

Not too sure if having a progress bar will make sense 100% of the time. What if the user is shuffling through the playlist? This doesn't seem to account for reverse order of playlists either. I don't see this as being as beneficial as we might think it will be. Open to other opinions however.

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.

What about only showing this when the order is "ascending" (with shuffling disabled)
We can figure out if that's useful for "reverse order" (I don't even know that exists)

But yup in this case this new components should only be displayed sometimes

@absidue absidue added PR: changes requested and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Jul 20, 2022
auto-merge was automatically disabled July 20, 2022 19:52

Head branch was pushed to by a user without write access

thx @PikachuEXE

Co-authored-by: PikachuEXE <pikachuexe@gmail.com>
@PrestonN PrestonN enabled auto-merge (squash) July 20, 2022 19:52
auto-merge was automatically disabled July 20, 2022 19:53

Head branch was pushed to by a user without write access

Co-authored-by: PikachuEXE <pikachuexe@gmail.com>
@PrestonN PrestonN enabled auto-merge (squash) July 20, 2022 19:53
auto-merge was automatically disabled July 20, 2022 20:03

Head branch was pushed to by a user without write access

@PrestonN PrestonN enabled auto-merge (squash) July 20, 2022 20:03
@makerio90
Copy link
Contributor Author

makerio90 commented Jul 20, 2022

What about only showing this when the order is "ascending" (with shuffling disabled) We can figure out if that's useful for "reverse order" (I don't even know that exists)

But yup in this case this new components should only be displayed sometimes

Im checking for !shuffleEnabled && !reversePlaylist

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.

Tested locally

Screen.Recording.2022-07-21.at.10.30.54.mov

@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 Aug 2, 2022
Copy link
Member

Choose a reason for hiding this comment

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

LGTM

@efb4f5ff-1298-471a-8973-3d47447115dc

@absidue or @ChunkyProgrammer one more approval needed. Review if u got some spare time.

@absidue
Copy link
Member

absidue commented Aug 16, 2022

When the page is between 900px and 1292px wide the progress bar will wrap around to a new line if the channel name or current index are "too long", which looks bad, ideally the progress bar would stay on the same line as the channel name and the numbers, just changing it's width to fit into the available space.
playlist-progress-bar

@absidue
Copy link
Member

absidue commented Aug 16, 2022

Okay so I realised that fixing that issue requires more CSS and changing the layout, so I'm happy getting this merged and I'll create a PR to fix that issue afterwards. That way I can also make sure that the CSS changes I want to make will work with the newer CSS on the development branch (I tried rebasing but the commits with the package.json, package-lock.json and yarn.lock files break rebasing).

@PrestonN PrestonN merged commit 4f9aa23 into FreeTubeApp:development Aug 16, 2022
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Aug 16, 2022
@makerio90 makerio90 deleted the patch-1 branch August 21, 2022 20:21
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.

None yet

5 participants