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.

Sign up for GitHub

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

Jump to bottom

Text align preferences added on Lyrics Mobile Server #147

Open

mbsrz1972 wants to merge 3 commits into quelea-projection:master from mbsrz1972:master

Contributor

mbsrz1972 commented Jun 4, 2019

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

mbsrz1972 added 2 commits

May 27, 2019 01:14


          Mobile Lyrics Server text-align options

f3e3efd


          formatting code

4ff8117

ArvidNy requested changes

View reviewed changes

Member

ArvidNy left a comment

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!


          polish lang update

e1dd527

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