Skip to content

Use Default Theme in Multiplayer#10399

Merged
thsparks merged 191 commits intomasterfrom
thsparks/theme_multiplayer
Feb 26, 2025
Merged

Use Default Theme in Multiplayer#10399
thsparks merged 191 commits intomasterfrom
thsparks/theme_multiplayer

Conversation

@thsparks
Copy link
Copy Markdown
Contributor

Themes impact multiplayer in a few places (though not many, since it primarily uses Tailwind). Notably, the settings dropdown and a few buttons. Actually supporting themes would be a big undertaking, so for now, I've just set it up to always load the default theme.

With themes, the presence buttons were getting dark backgrounds because they were disabled. This looked odd, so I've added a tailwind class to force them back to their white backgrounds instead.

I also had to fix a bug in our multiplayer serve that assumed all webconfig values were strings (no longer the case with ocv).

Upload Target: https://arcade.makecode.com/app/bd228f8101401d913f282b0c3585b210d67a903b-d766c6703c--multiplayer

Fixes microsoft/pxt-arcade#6722

…d a new pxtrcdeps (pxt react common dependencies) min.js file which includes (for now) just the DOMPurify code. Then I added a reference to it in skillmap.
…vars were used previously, but put back all the hardcoded values.
… but the little numbers on the right aren't themed to contrast in dark theme, and I was struggling to get them working. So just hard-code for now.
@thsparks thsparks requested a review from a team February 26, 2025 00:52
Copy link
Copy Markdown
Contributor

@srietkerk srietkerk left a comment

Choose a reason for hiding this comment

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

Looks good! One thing I saw on the upload target is that since the button colors are changed to the default, the underline color for Join and Host Game are now different from the button. Not blocking, but I wanted to bring it up in case it was something we wanted to change.

Live:
image

New:
image


for (const key of Object.keys(wc)) {
if (wc[key]?.startsWith("/") && wc[key]?.indexOf("worker") == -1) {
if (typeof wc[key] === "string" && wc[key]?.startsWith("/") && wc[key]?.indexOf("worker") == -1) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm, this makes me a bit worried that we have this constraint other places.. I'll have to take a look around to make sure I'm not breaking anything else. Thanks for fixing this here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No worries. TBH I don't think ocv is at fault here. Just an unsafe assumption in the code. ocv wasn't actually the first boolean value added to the config, just the first one that was actually set at this point.

@thsparks
Copy link
Copy Markdown
Contributor Author

One thing I saw on the upload target is that since the button colors are changed to the default, the underline color for Join and Host Game are now different from the button. Not blocking, but I wanted to bring it up in case it was something we wanted to change.

Good catch. Turns out there's some primary/secondary/tertiary color usage that I didn't notice. I'll update those.

@thsparks thsparks merged commit ab72ff2 into master Feb 26, 2025
6 checks passed
@thsparks thsparks deleted the thsparks/theme_multiplayer branch February 26, 2025 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Themes: Multiplayer needs theme support

2 participants