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

Make sure the SettingsStore is ready to load the theme before loading it #5630

Merged
merged 4 commits into from Nov 27, 2017

Conversation

@turt2live
Copy link
Member

turt2live commented Nov 17, 2017

Required PRs

Description

This PR contains a fix for #5623 in addition to one other fix. The config has to be loaded (and set) before calling SettingsStore otherwise all settings at the config level are ignored. Further, it helps to call getValue instead of getValueAt if we want the config to be respected.

The other included fix is to make sure the config is loaded prior to checking the user agent. This is so the default theme check reacts correctly, as per the above.

Visibility of what is going on in the diff might be a bit unclear, so going commit by commit may be better. The summary is the flow is now: load config -> check if mobile -> set theme -> rest of loading stuff

turt2live added 3 commits Nov 17, 2017
This is to ensure that when we make a call to get the theme we'll have the SdkConfig available.

Signed-off-by: Travis Ralston <travpc@gmail.com>
This is so the `config` level is respected.

Signed-off-by: Travis Ralston <travpc@gmail.com>
Signed-off-by: Travis Ralston <travpc@gmail.com>
@turt2live

This comment has been minimized.

Copy link
Member Author

turt2live commented Nov 21, 2017

Paging @ara4n

Copy link
Member

dbkr left a comment

lgtm, but can we add a comment on why we're passing the config into react-sdk twice (once to SdkConfig and once as a prop to MatrixChat)?

Signed-off-by: Travis Ralston <travpc@gmail.com>
@dbkr
dbkr approved these changes Nov 27, 2017
@dbkr dbkr merged commit a346cf3 into vector-im:develop Nov 27, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.