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

Linux tray icon theme settings regression fix #392

Merged
merged 2 commits into from
Dec 12, 2016

Conversation

jnugh
Copy link
Contributor

@jnugh jnugh commented Dec 10, 2016

Before submitting, please confirm you've

Please provide the following information:

Summary
This fixes a regression on the settings page that was introduced by upgrading react bootstrap (upgrading to the new form syntax).
On Linux the user can choose from a dark and a light theme for the tray icon. This setting was a select element which was transformed into a checkbox during react bootstrap upgrade.
The user was therefore not able to choose a different theme, when using the checkbox control a bool value was written to the config file which kept the app from opeining up on the next start.

This fix shows the dropdown again, allows to set sane values for the theme config and has a fallback when an invalid value has already been written to the config file - so that the application starts again.

Test Cases

Additional Notes

@jasonblais
Copy link
Contributor

Wow, this is a great find @jnugh.

@yuya-oc can you help test this PR since we'll want this in for RC3?

@yuya-oc
Copy link
Contributor

yuya-oc commented Dec 11, 2016

That's my mistake, thanks for fixing and adding fallback!

@jasonblais Sure, it correctly changes theme. But need git-rebase.

@yuya-oc yuya-oc changed the base branch from master to release-3.5 December 11, 2016 04:51
Copy link
Contributor

@yuya-oc yuya-oc left a comment

Choose a reason for hiding this comment

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

To add these changes into v3.5, please rebase to release-3.5 branch.

@jnugh
Copy link
Contributor Author

jnugh commented Dec 11, 2016

Here we go, just reapplied the commits on a fresh 3.5 branch, works for me 😄

@yuya-oc
Copy link
Contributor

yuya-oc commented Dec 12, 2016

I tested again and it works well. Thanks!

@yuya-oc yuya-oc merged commit 1f11e8e into mattermost:release-3.5 Dec 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants