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

Update classic/style.css with TabWidget, TrackView, PianoView and Fader colors #3665

Merged
merged 4 commits into from Jul 3, 2017

Conversation

Projects
None yet
4 participants
@PhysSong
Member

PhysSong commented Jun 25, 2017

It fixes #3652 by adding TabWidget color settings to classic/style.css.
@Umcaruje @RebeccaLaVie If you don't like this color, please change it.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Jul 2, 2017

Member

@Umcaruje Now you can push commits on my fork. Please change colors appropriately.

Member

PhysSong commented Jul 2, 2017

@Umcaruje Now you can push commits on my fork. Please change colors appropriately.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Jul 2, 2017

Member

@Umcaruje @RebeccaLaVie If you don't like this color, please change it.

@PhysSong Can you please provide a preview of this PR? That would make the decision a bit quicker.

Member

tresf commented Jul 2, 2017

@Umcaruje @RebeccaLaVie If you don't like this color, please change it.

@PhysSong Can you please provide a preview of this PR? That would make the decision a bit quicker.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Jul 2, 2017

Member

@tresf I can't provide preview right now. Currently colors are the same as default theme.

Member

PhysSong commented Jul 2, 2017

@tresf I can't provide preview right now. Currently colors are the same as default theme.

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Jul 2, 2017

Member

I'll look into this today

Member

Umcaruje commented Jul 2, 2017

I'll look into this today

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Jul 2, 2017

Member

I updated the colors:
image

I also ran a diff between the two themes, and updated some other changes in the classic theme that we missed, so the files now should have the identical rules, just different colors.

Member

Umcaruje commented Jul 2, 2017

I updated the colors:
image

I also ran a diff between the two themes, and updated some other changes in the classic theme that we missed, so the files now should have the identical rules, just different colors.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Jul 2, 2017

Member

@Umcaruje Great job! Thanks a lot.
Is it ready for merge?

Member

PhysSong commented Jul 2, 2017

@Umcaruje Great job! Thanks a lot.
Is it ready for merge?

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Jul 2, 2017

Member

I'll leave this open for 12 hours if anyone wants to chime in, I'll merge afterwards.

Member

Umcaruje commented Jul 2, 2017

I'll leave this open for 12 hours if anyone wants to chime in, I'll merge afterwards.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Jul 2, 2017

Member

I see a lot of unnecessary formatting changes. Can we just change the lines that are needed for the PR? data/themes/default/style.css shouldn't even be in this PR. The more lines we touch, the harder it is to rebase onto master.

Member

tresf commented Jul 2, 2017

I see a lot of unnecessary formatting changes. Can we just change the lines that are needed for the PR? data/themes/default/style.css shouldn't even be in this PR. The more lines we touch, the harder it is to rebase onto master.

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Jul 2, 2017

Member

The thing is that I tried to make the formatting consistent throughout the file, cause it was all over the place, and using spaces instead of tabs. I am not aware of any theme changes on master, so I don't think rebasing this would be a problem.

Member

Umcaruje commented Jul 2, 2017

The thing is that I tried to make the formatting consistent throughout the file, cause it was all over the place, and using spaces instead of tabs. I am not aware of any theme changes on master, so I don't think rebasing this would be a problem.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Jul 2, 2017

Member

@tresf Yes. default/style.css shouldn't in this PR.
We need only fix difference between two css file. Formatting is what @Umcaruje should do after this PR is merged.

Member

PhysSong commented Jul 2, 2017

@tresf Yes. default/style.css shouldn't in this PR.
We need only fix difference between two css file. Formatting is what @Umcaruje should do after this PR is merged.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Jul 2, 2017

Member

Formatting is what @Umcaruje should do after this PR is merged.

Formatting should be targeted at master, really and we have an initiative to do formatting cleanup anyway. Stable branches are not actively developed, so making the source more readable serves no purpose.

Member

tresf commented Jul 2, 2017

Formatting is what @Umcaruje should do after this PR is merged.

Formatting should be targeted at master, really and we have an initiative to do formatting cleanup anyway. Stable branches are not actively developed, so making the source more readable serves no purpose.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Jul 2, 2017

Member

@tresf @Umcaruje If there's no objection, I'll revert last three commits and andd a commit to remove unnecessary formatting change.
I will also provide diff file between re-formatted css and one without formatting change. @Umcaruje You will be able to use the diff for further formatting.

Member

PhysSong commented Jul 2, 2017

@tresf @Umcaruje If there's no objection, I'll revert last three commits and andd a commit to remove unnecessary formatting change.
I will also provide diff file between re-formatted css and one without formatting change. @Umcaruje You will be able to use the diff for further formatting.

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Jul 2, 2017

Member

@tresf @Umcaruje If there's no objection, I'll revert last three commits and andd a commit to remove unnecessary formatting change.
I will also provide diff file between re-formatted css and one without formatting change. @Umcaruje You will be able to use the diff for further formatting.

Or I can just re-commit the 5 lines I changed tommorow, you two make good points and I'll fix this :)

Member

Umcaruje commented Jul 2, 2017

@tresf @Umcaruje If there's no objection, I'll revert last three commits and andd a commit to remove unnecessary formatting change.
I will also provide diff file between re-formatted css and one without formatting change. @Umcaruje You will be able to use the diff for further formatting.

Or I can just re-commit the 5 lines I changed tommorow, you two make good points and I'll fix this :)

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Jul 2, 2017

Member

@Umcaruje I can do it soon. Would the commits be squashed on merge?
@tresf Thanks for your feedback!

Member

PhysSong commented Jul 2, 2017

@Umcaruje I can do it soon. Would the commits be squashed on merge?
@tresf Thanks for your feedback!

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Jul 2, 2017

Member

Would the commits be squashed on merge?

Yeah, thanks for working on this. Don't forget the changes in Fader and the track background and the piano widget :)

Member

Umcaruje commented Jul 2, 2017

Would the commits be squashed on merge?

Yeah, thanks for working on this. Don't forget the changes in Fader and the track background and the piano widget :)

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Jul 2, 2017

Member

Would the commits be squashed on merge?

Yes, that is our default option, so they'd be squashed unless asked otherwise.

Member

tresf commented Jul 2, 2017

Would the commits be squashed on merge?

Yes, that is our default option, so they'd be squashed unless asked otherwise.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Jul 2, 2017

Member

I reverted formatting changes. I also attached the diff for further formatting.

Member

PhysSong commented Jul 2, 2017

I reverted formatting changes. I also attached the diff for further formatting.

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Jul 3, 2017

Member

Travis CI is failing, but it isn't a problem obviously.
See this.

Member

PhysSong commented Jul 3, 2017

Travis CI is failing, but it isn't a problem obviously.
See this.

@zonkmachine

This comment has been minimized.

Show comment
Hide comment
@zonkmachine

zonkmachine Jul 3, 2017

Member

Travis CI is failing, but it isn't a problem obviously.

I restarted the failing build.

Member

zonkmachine commented Jul 3, 2017

Travis CI is failing, but it isn't a problem obviously.

I restarted the failing build.

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Jul 3, 2017

Member

Yeah, this should've stayed, not a formatting change

Member

Umcaruje commented on data/themes/classic/style.css in eaf4cf0 Jul 3, 2017

Yeah, this should've stayed, not a formatting change

@PhysSong

This comment has been minimized.

Show comment
Hide comment
@PhysSong

PhysSong Jul 3, 2017

Member

@Umcaruje Now fixed.

Member

PhysSong commented Jul 3, 2017

@Umcaruje Now fixed.

@Umcaruje

This comment has been minimized.

Show comment
Hide comment
@Umcaruje

Umcaruje Jul 3, 2017

Member

@PhysSong thank you very much for your work on this, this is ready to merge 👍

Member

Umcaruje commented Jul 3, 2017

@PhysSong thank you very much for your work on this, this is ready to merge 👍

@Umcaruje Umcaruje changed the title from Add TabWidget color settings to classic/style.css to Update classic/style.css with TabWidget, TrackView, PianoView and Fader colors Jul 3, 2017

@Umcaruje Umcaruje merged commit 494714b into LMMS:stable-1.2 Jul 3, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

PhysSong added a commit to PhysSong/lmms that referenced this pull request Jul 8, 2017

Update classic/style.css with TabWidget, TrackView, PianoView and Fad…
…er colors (#3665)

* Add TabWidget color settings to classic/style.css

* update the classic theme

* Revert unnecessary formattings

* Fix style.css

PhysSong added a commit to PhysSong/lmms that referenced this pull request Jul 8, 2017

Update classic/style.css with TabWidget, TrackView, PianoView and Fad…
…er colors (#3665)

* Add TabWidget color settings to classic/style.css

* update the classic theme

* Revert unnecessary formattings

* Fix style.css
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment