Add publish plain text button

tranxuanthang / lrcget

Utility for mass-downloading LRC synced lyrics for your offline music library.

MIT License

540 stars 20 forks source link

Add publish plain text button #35

Closed shipurjan closed 5 months ago

shipurjan commented 5 months ago

Adds a button allowing for publishing plain text lyrics as requested in #34 Includes a basic linter, that checks if:

plain text lyrics doesn't have lines starting with [
first line isn't empty
there are no 2 consecutive empty lines

shipurjan commented 5 months ago

Commit a90f4c5 fixed editor song title responsiveness

Before that it could wrap

Amended a commit with force-push because I forgot to delete text LONG TEXT TO TEST BLAH BLAH BLAH BLAH BLAH BLAH BLAH BLAH BLAH BLAH BLAH BLAH BLAH BLAH BLAH when testing

tranxuanthang commented 5 months ago

Hey, thank you for your great effort! I've just got back from vacation and are reviewing your PR right now. Overall, this is a very clean and really good first PR. I highly appreciate that you added a plain lyrics linter for this.

There is currently one thing I think that we could improve: two separate buttons for publishing might be a little bit confusing for users. I'm thinking of some alternatives:

Display a single "Publish" button, but with a dropdown icon on the right side for showing more options. I imagine it will be similar to this component from flowbite.
Or even better, when users trying to publish a lyrics that does not have any timestamp brackets, we can show a small modal confirming if they want to publish it as plain lyrics only, or go back and continue synchronize the lyrics.

What do you think about this?

shipurjan commented 5 months ago

Hi, I personally think that the dropdown button solution would be simpler (both to maintain and less confusing for users). They could see right away that there is some additional publish method, whereas in the second method it seems to be more hidden. But that's just my opinion, the final decision is up to you :)

tranxuanthang commented 5 months ago

@shipurjan I agree with your perspective. Would you like to incorporate these changes into this PR, or would you prefer merging this one first and creating a separate PR specifically for this improvement?

shipurjan commented 5 months ago

I can integrate it into this PR

shipurjan commented 5 months ago

Okay I added a dropdown button, and also a live icon indication whether they can publish the current lyrics

Feel free to change anything if you don't like how it looks or works

tranxuanthang commented 5 months ago

Thank you for the update! You did great on the UI elements. I think it is good to merge this PR now.

Looking forward to more of your contributions in the future!