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

Text align preferences added on Lyrics Mobile Server #147

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

Conversation

mbsrz1972
Copy link
Contributor

Hi, I found it usefull for my needs so maybe you will. + Little polish language update

Copy link
Member

@ArvidNy ArvidNy left a comment

Choose a reason for hiding this comment

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

Thanks, looks like a good addition to me! Haven't had the chance to test run this yet but the code looks alright to me. Personally I think I would prefer a separate function to be called from onchange on line 310 instead of having the two lines of JavaScript in the HTML code, but I suppose that's just a matter of preference.

However, I would prefer if you split the two changes (translation and new feature) into two pull requests instead. It's not a big deal, but it's always better to keep these kinds of changes apart so that we instantly can merge what does not need to be reviewed by us (translations) and comment on what has to be reviewed (new features).

Many thanks for the PR!

@ArvidNy
Copy link
Member

ArvidNy commented Sep 30, 2019

You seem to have misunderstood me about the labels, but if you resolve the conflict with the labels I can merge it. Just make sure to make a separate issue for updates to the language file in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants