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

Better-Lyrics Styling for Synced-Lyrics #2554

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kimjammer
Copy link

@kimjammer kimjammer commented Oct 26, 2024

This PR reimplements the CSS styles and animations for synced-lyrics so that it is more like the lyrics view for Spotify or the YT music mobile apps.

Yesterday, I found boidushya/better-lyrics and while it's great, it's a web browser extension so you can't install it into this app. So, I spent an afternoon porting the CSS over from that into this pr.

This PR is a draft because:

  1. The code style & css variable names need to be updated (if you want) - I left the original css variable names so it would be clear what's from better-lyrics
  2. The new CSS overrides the old "Line effect" settings and that code would need to be removed
  3. better-lyrics is GPLv3 and idk how the "same license" clause applies + I want to get @boidushya 's blessing for this port
    (Assuming boidushya has ping on mention turned on: Would you be okay with this few dozen line CSS port? Would you mind if I eventually (not this pr) ported the romanization and translation features as well?)

What do you think? I'm open for discussion.

Screen.Recording.2024-10-26.194514.mp4

@boidushya
Copy link

Love this! You have my full support for the port. If you want - happy to create a channel for this port in our discord so we can keep things in sync :))

@Dzheremi2
Copy link
Contributor

I love the original synced-lyrics line effect. You should probaly make enabling Better-lyrics style optional. Better-lyrics line effect is too big to me.

@ArjixWasTaken
Copy link
Contributor

Ideally this should be merged after #2383

@ArjixWasTaken
Copy link
Contributor

ArjixWasTaken commented Oct 27, 2024

@kimjammer no need to "port" romanization or translation features from someone else's code
and there is an open discussion for that here

they are on my TODO list for after I am done with my current PR
(not saying that you can't do that urself, but it is smth simple enough to not need to be copied over)

@kimjammer
Copy link
Author

Ideally this should be merged after #2383

I would love for #2383 to be merged as well, but I didn't want to wait that long lol (and I don't want you to feel pressured). I actually did most of the dev for this pr on your branch, so it will still work when your pr gets merged in!

@kimjammer
Copy link
Author

@kimjammer no need to "port" romanization or translation features from someone else's code and there is an open discussion for that here

they are on my TODO list for after I am done with my current PR (not saying that you can't do that urself, but it is smth simple enough to not need to be copied over)

I should have clarified: the "port" is referring to the CSS, I don't really care where the romanization or the translations come from. For reference, the better lyrics CSS for the romanization & translation
image

@kimjammer
Copy link
Author

kimjammer commented Oct 28, 2024

Alrighty! PR is ready for review - (none of the repo contributors have even said if they think this pr is a good idea yet lol)

  1. Code has been cleaned up ✅
  2. New styling has been implemented as "Fancy" line effect & the 3 old line effects have been reimplemented ✅
  3. Permission granted from original author ✅

I took a random guess at how i18n was supposed to be done, if there's more lmk.

Screenshot 2024-10-27 232510
Screenshot 2024-10-27 232524
Screenshot 2024-10-27 232536

@kimjammer kimjammer marked this pull request as ready for review October 28, 2024 03:33
@JellyBrick JellyBrick added the enhancement New feature or request label Oct 28, 2024
@Rairof
Copy link

Rairof commented Oct 31, 2024

#2554 (comment)
This is great, exactly what I was looking to suggest!
A suggestion-
Can you also make just the 3 lyrics align and show up while making the current syncing lyric line in the middle and make 1 lyrics line above and below and making the rest invisible to not make the lyrics page look crowded?
Pic in reference-
image

@ArjixWasTaken
Copy link
Contributor

ArjixWasTaken commented Oct 31, 2024

I think this will get out of hand if everyone who has an opinion makes a request.
Stuff that is purely about the appearance of smth should fall under community made themes.

#2540

Disclaimer: I am not a maintainer, so I don't have the final say, this is merely a suggestion to improve the user experience overall and not in just some areas. I invite anyone that wants to make further changes to converse in the linked issue.

Adding a settings toggle to switch between the old and new CSS in order to make everyone happy is a spaghetti solution.
That means you'd either have spaghetti or force ur stylistic choices on everyone else.
In order to satisfy everyone, it would be better if the styling was kept to being simple, and we instead allow the community to author styles and share them w/o having to force them on anyone.

I hope I didn't come off as being dismissive or rude, I just really think we should not rush things.

@kimjammer
Copy link
Author

Maintainers, lmk if you have opinions or changes to request.
As far as #2540 goes, the CSS animations used here require changes to the text rendering code, which obviously can't be done just in CSS. Maybe think of this PR as being closer to "bringing feature parity (pretty default synced lyrics) from mobile apps" rather than "Adding CSS choices".
PR is ready to be merged on my end.

Copy link
Collaborator

@JellyBrick JellyBrick left a comment

Choose a reason for hiding this comment

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

I'll merge this PR after #2383 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants