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

De-dup theming code #11445

Merged
merged 2 commits into from Nov 21, 2019
Merged

De-dup theming code #11445

merged 2 commits into from Nov 21, 2019

Conversation

@dbkr
Copy link
Member

dbkr commented Nov 20, 2019

Following on from matrix-org/matrix-react-sdk#3637
this removes the code dealing with themes in vector/index.js and uses the
code from react-sdk. The two did almost exactly the same thing but in
subtley different ways.

This code can be incredibly subtle though, so doing this a separate
PR.

Based on #11442

dbkr added 2 commits Nov 20, 2019
A thesis presented in two parts. This part has the absolute minimum
logic changes to the themeing code in vector/index.js because I know
how subtle and fragile this code is. However, it also looks like it's
completely duplicated from react-sdk, so in the next part I'm going
to remove that logic and make it use the logic in react-sdk, then we
can see what breaks.

Requires matrix-org/matrix-react-sdk#3637
Following on from matrix-org/matrix-react-sdk#3637
this removes the code dealing with themes in vector/index.js and uses the
code from react-sdk. The two did almost exactly the same thing but in
subtley different ways.

This code can be incredibly subtle though, so doing this a separate
PR.
@dbkr dbkr requested a review from vector-im/riot-web Nov 20, 2019
dbkr added a commit to matrix-org/matrix-react-sdk that referenced this pull request Nov 20, 2019
This was only used by vector/index.js, in the code removed by
vector-im/riot-web#11445

React SDK does a very similar thing in setTheme but also gets the
rest of the custom theme name.

Requires vector-im/riot-web#11445
@dbkr dbkr mentioned this pull request Nov 20, 2019
@turt2live

This comment has been minimized.

Copy link
Member

turt2live commented Nov 20, 2019

I'm not sure it's safe to consider this code duplicated - the code is waiting for the CSS to load so it can set the theme without turning into a pile of no-css sadness.

@dbkr

This comment has been minimized.

Copy link
Member Author

dbkr commented Nov 20, 2019

Yep, but the code in setTheme attempts to do that too.

@dbkr dbkr added this to In Review in Workflow Nov 20, 2019
Copy link
Contributor

bwindels left a comment

looks like theming stuff

@dbkr dbkr merged commit 8b2ca56 into develop Nov 21, 2019
5 checks passed
5 checks passed
buildkite/riot-web Build #1342 passed (3 minutes, 32 seconds)
Details
buildkite/riot-web/eslint-lint Passed (43 seconds)
Details
buildkite/riot-web/i18n Passed (2 minutes, 19 seconds)
Details
buildkite/riot-web/karma-tests Passed (3 minutes, 20 seconds)
Details
buildkite/riot-web/pipeline Passed (6 seconds)
Details
Workflow automation moved this from In Review to In Test Nov 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.