Skip to content

Conversation

shipurjan
Copy link
Contributor

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
Copy link
Contributor Author

Commit a90f4c5 fixed editor song title responsiveness
image

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

@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
Copy link
Contributor Author

I can integrate it into this PR

@shipurjan
Copy link
Contributor Author

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
Copy link
Owner

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!

@tranxuanthang tranxuanthang merged commit 47ef03e into tranxuanthang:main Jan 18, 2024
@shipurjan shipurjan deleted the add-publish-plain-text-button branch January 18, 2024 18:09
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.

2 participants