[Copilot] microsoft/pxt#10391 — Add Theme Support 🎨 - #42
Open
IanMatthewHuff wants to merge 1 commit into
Open
Conversation
IanMatthewHuff
added
the
pr-sxs-human-evals/comparison-pr
There was a problem hiding this comment.
Pull request overview
Adds runtime-switchable color theme support across the MakeCode web editor (Arcade-first) and the Skillmap app by introducing a theme manager, a theme picker UI, and migrating many hard-coded colors/LESS variables to CSS custom properties.
Changes:
- Introduces
ThemeManager+ThemePickerModal/ preview cards to switch CSS variables at runtime and persist the user’s selected theme. - Updates webapp + skillmap UI components to use semantic color classes (
neutral,tertiary, etc.) aligned with the new theme variables. - Reworks large portions of styling (theme + react-common + skillmap) to rely on
--pxt-*CSS variables and adds bundling support for theme JSON + override CSS.
Show a summary per file
| File | Description |
|---|---|
| webapp/src/sidepanel.tsx | Button class updates for theming |
| webapp/src/serial.tsx | Serial UI theming adjustments |
| webapp/src/scriptmanager.tsx | Neutral button styling updates |
| webapp/src/projects.tsx | Theme picker menu wiring |
| webapp/src/monaco.tsx | Monaco base theme from color theme |
| webapp/src/lang.tsx | Neutral button styling |
| webapp/src/gitjson.tsx | Neutral button/segment styling |
| webapp/src/extensionsBrowser.tsx | Neutral button styling |
| webapp/src/core.ts | HC logic integrates theme selection |
| webapp/src/container.tsx | Theme picker menu wiring |
| webapp/src/components/tutorial/TutorialValidationErrorMessage.tsx | Button class updates for theming |
| webapp/src/components/tutorial/TutorialContainer.tsx | Back button styling |
| webapp/src/components/soundEffectEditor/SoundPreview.tsx | Remove hard-coded SVG strokes |
| webapp/src/components/soundEffectEditor/SoundGallery.tsx | Remove hard-coded SVG strokes |
| webapp/src/components/assetEditor/assetSidebar.tsx | Button class updates for theming |
| webapp/src/components/assetEditor/assetPalette.tsx | Button class updates for theming |
| webapp/src/codecard.tsx | Tutorial progress label class change |
| webapp/src/auth.ts | Persist themeId preference |
| webapp/src/app.tsx | Load/apply theme + show picker modal |
| webapp/src/accessibility.tsx | Accessibility menu: theme picker |
| webapp/public/index.html | Adds .pxt-theme-root |
| theme/webusb.less | Uses CSS var for border color |
| theme/tutorial.less | Theme variable migration |
| theme/tutorial-sidebar.less | Theme variable migration |
| theme/tooltips.less | Theme variable migration |
| theme/toolbox.less | Theme variable migration |
| theme/timeMachine.less | Theme variable migration |
| theme/themes/pxt/views/card.variables | Theme var-based card variables |
| theme/themes/pxt/views/card.overrides | Theme var-based card overrides |
| theme/themes/pxt/modules/modal.overrides | Theme var-based modal overrides |
| theme/themes/pxt/globals/site.variables | Removes LESS colors; warns about SUI vars |
| theme/themes/pxt/elements/segment.variables | Removes segment LESS vars |
| theme/themes/pxt/elements/loader.variables | Removes loader LESS vars |
| theme/themes/pxt/collections/menu.variables | Uses CSS vars for inverted hover |
| theme/themes/pxt/collections/menu.overrides | Header/menu theming via vars |
| theme/soundeffecteditor.less | Theme variable migration |
| theme/serial.less | Theme variable migration |
| theme/semantic-ui-overrides.less | New: semantic UI overrides via vars |
| theme/pxt.less | Imports new overrides |
| theme/music-editor/TrackSelector.less | Theme variable migration |
| theme/music-editor/Staff.less | Theme variable migration |
| theme/music-editor/PlaybackControls.less | Theme variable migration |
| theme/music-editor/MusicEditor.less | Theme variable migration |
| theme/monaco.less | Theme variable migration |
| theme/light.less | Blockly bg uses theme vars |
| theme/image-editor/imageEditor.less | Partial theming + TODO |
| theme/image-editor/button.less | Focus border uses theme var |
| theme/image-editor/bottomBar.less | Theme variable migration |
| theme/home.less | Home UI theme variable migration |
| theme/highcontrast.less | HC uses theme vars |
| theme/greenscreen.less | Theme variable migration |
| theme/github.less | Theme variable migration |
| theme/errorList.less | Theme variable migration |
| theme/diff.less | Theme variable migration |
| theme/debugger.less | Theme variable migration |
| theme/common-components.less | Theme variable migration |
| theme/color-themes/README.md | New: color theme format docs |
| theme/color-themes/overrides/high-contrast-overrides.css | New: HC overrides CSS |
| theme/color-themes/high-contrast.json | New: HC theme definition |
| theme/blockly-core.less | Theme variable migration |
| theme/asset-editor.less | Theme variable migration |
| skillmap/src/styles/skillcard.css | Skillmap uses --pxt-* vars |
| skillmap/src/styles/makecode-editor.css | Skillmap uses --pxt-* vars |
| skillmap/src/styles/infopanel.css | Skillmap uses --pxt-* vars |
| skillmap/src/styles/graphnode.css | Focus outline uses theme var |
| skillmap/src/styles/carousel.css | Skillmap uses --pxt-* vars |
| skillmap/src/store/reducer.ts | Adds theme picker state |
| skillmap/src/lib/skillMapParser.ts | Default colors updated for vars |
| skillmap/src/lib/authClient.ts | Reads themeId pref |
| skillmap/src/components/RewardActions.tsx | Button class updates for theming |
| skillmap/src/components/makecodeFrame.tsx | Sends setcolortheme to iframe |
| skillmap/src/components/HeaderBar.tsx | Theme picker in header menu |
| skillmap/src/components/ActivityActions.tsx | Button class updates for theming |
| skillmap/src/arcade.css | Removes legacy arcade CSS vars |
| skillmap/src/App.tsx | Loads theme + shows theme picker |
| skillmap/src/App.css | Skillmap CSS migrates to vars |
| skillmap/src/actions/types.ts | Adds theme picker actions |
| skillmap/src/actions/dispatch.ts | Adds theme picker dispatchers |
| skillmap/public/index.html | Adds .pxt-theme-root + pxtrcdeps |
| react-common/styles/theming/ThemePickerModal.less | New: theme picker styles |
| react-common/styles/theming/base-theme.less | New: base CSS variables |
| react-common/styles/share/share.less | Theme var migration |
| react-common/styles/react-common.less | Imports new theming styles |
| react-common/styles/react-common-variables.less | Removes LESS color vars in favor of CSS vars |
| react-common/styles/profile/UserAvatarDropdown.less | Theme var migration |
| react-common/styles/profile/profile.less | Theme var migration |
| react-common/styles/onboarding/TeachingBubble.less | Theme var migration |
| react-common/styles/language/language.less | Formatting-only change |
| react-common/styles/extensions/ExtensionCard.less | Theme var migration |
| react-common/styles/controls/VerticalSlider.less | Focus border uses theme var |
| react-common/styles/controls/VerticalResizeContainer.less | Theme var migration |
| react-common/styles/controls/Tree.less | Theme var migration |
| react-common/styles/controls/Textarea.less | Theme var migration |
| react-common/styles/controls/RadioButtonGroup.less | Theme var migration |
| react-common/styles/controls/ProgressBar.less | Theme var migration |
| react-common/styles/controls/Modal.less | Theme var migration |
| react-common/styles/controls/MenuDropdown.less | Theme var migration |
| react-common/styles/controls/Input.less | Theme var migration |
| react-common/styles/controls/EditorToggle.less | Theme var migration |
| react-common/styles/controls/Dropdown.less | Theme var migration |
| react-common/styles/controls/DraggableGraph.less | Theme var migration |
| react-common/styles/controls/Card.less | Theme var migration |
| react-common/styles/controls/Button.less | Theme var migration + removes old variants |
| react-common/components/theming/ThemePreview.tsx | New: theme preview renderer |
| react-common/components/theming/ThemePickerModal.tsx | New: theme picker modal |
| react-common/components/theming/themeManager.ts | New: theme switching + subscribers |
| react-common/components/theming/ThemeCard.tsx | New: theme card component |
| react-common/components/share/ShareInfo.tsx | Neutral button styling |
| react-common/components/profile/SignInModal.tsx | Provider button class change |
| react-common/components/Notification.tsx | Info notification color class |
| react-common/components/extensions/DeleteConfirmationModal.tsx | Formatting-only change |
| react-common/components/controls/TeachingBubble.tsx | Uses tertiary buttons |
| react-common/components/controls/Modal.tsx | Close button class change |
| pxtservices/editorDriver.ts | Adds setColorTheme request helper |
| pxtlib/auth.ts | Adds themeId to preferences |
| pxteditor/editorcontroller.ts | Handles setcolortheme message |
| localtypings/pxteditor.d.ts | Adds theme picker + message types |
| localtypings/pxtarget.d.ts | Adds theme fields + colorThemeMap |
| gulpfile.js | Builds pxtrcdeps (dompurify) |
| cli/cli.ts | Bundles theme JSON + override CSS |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 117/121 changed files
- Comments generated: 8
Comment on lines
+14
to
+31
| private currentTheme: pxt.ColorThemeInfo; | ||
| private subscribers: ThemeChangeSubscriber[]; | ||
| private document: Document; | ||
|
|
||
| private constructor(doc: Document) { | ||
| this.document = doc; | ||
| } | ||
|
|
||
| public static getInstance(doc: Document = document): ThemeManager { | ||
| if (!ThemeManager.instances.has(doc)) { | ||
| ThemeManager.instances.set(doc, new ThemeManager(doc)); | ||
| } | ||
| return ThemeManager.instances.get(doc); | ||
| } | ||
|
|
||
| public getCurrentColorTheme(): Readonly<pxt.ColorThemeInfo> { | ||
| return this.currentTheme; | ||
| } |
There was a problem hiding this comment.
I think the initialization happens in time so this is not a real error
Comment on lines
+20
to
+22
| --pxt-tertiary-background: #909090; | ||
| --pxt-tertiary-background-hover: "#777777"; | ||
| --pxt-tertiary-foreground: #ffffff; |
Comment on lines
+285
to
+292
| &.gray, &.grey, &.neutral { | ||
| background-color: var(--pxt-neutral-backgroundbackground2) !important; | ||
| color: var(--pxt-neutral-background-foreground2) !important; | ||
|
|
||
| &.inverted { | ||
| background-color: var(--pxt-neutral-background-foreground2) !important; | ||
| color: var(--pxt-neutral-background-background2) !important; | ||
| } |
Comment on lines
139
to
141
| padding: .5rem 1rem; | ||
| border-bottom: 2px solid darken(desaturate(@editorToolsBackground, 60%), 10%); | ||
| border-bottom: var(--pxt-target-stencil3); | ||
| user-select: none; |
| @conflictHeaderColor: black; | ||
| @diffUpdateInlineAddedOutline: 1px solid var(--pxt-neutral-stencil3); // #ccc | ||
|
|
||
| @conflictHeaderColor: var(pxt-neutral-background3); // black; |
| <sui.Item className={`${targetTheme.invertedMenu ? `inverted` : ''} menu`} role="menuitem" icon="upload" text={lf("Import Project")} onClick={this.importProjectDialog} /> | ||
| {targetTheme.selectLanguage ? <sui.Item className={`${targetTheme.invertedMenu ? `inverted` : ''} menu`} role="menuitem" icon="xicon globe" text={lf("Select Language")} onClick={this.showLanguagePicker} /> : undefined} | ||
| {targetTheme.highContrast ? <sui.Item className={`${targetTheme.invertedMenu ? `inverted` : ''} menu`} role="menuitem" text={highContrast ? lf("High Contrast Off") : lf("High Contrast On")} onClick={this.toggleHighContrast} /> : undefined} | ||
| {targetTheme.defaultColorTheme ? <sui.Item className={`${targetTheme.invertedMenu ? `inverted` : ''} menu`} role="menuitem" icon="paint brush" text={("Select Theme")} onClick={this.showThemePicker} /> : undefined} |
Comment on lines
+2321
to
+2323
| // Strip leading slashes, convert \ to /, and lowercase the path | ||
| const combinedPath = path.join(themeFileDir, overrideFile); | ||
| let cssText = fs.readFileSync(combinedPath, "utf8"); |
Comment on lines
772
to
+776
| const serialTheme = pxt.appTarget.serial && pxt.appTarget.serial.editorTheme; | ||
|
|
||
| // TODO : Integrate serialTheme & App Color Themes properly. | ||
| serialTheme.lineColors = [ | ||
| "var(--pxt-colors-blue-background)", |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This is a first pass at adding color theme support for arcade (and other targets in the future).
See the theme README.md file (
theme\color-themes\README.md) for details on what each theme variable is for. They are loosely based on fluentUI, though of course, we have a much smaller set to deal with.Upload Target: https://arcade.makecode.com/app/8ff7826fe1b820f3793c301c11dabf0ce5bcfdc0-0927214d31
Key Areas
Most of this change is just updating colors, but here are a few spots that deserve special attention:
react-common\components\theming\themeManager.ts- this is what handles actual theme-related operationscli\cli.ts- this is where theme json is added to the target bundlewebapp\src\app.tsx&pxtlib\auth.ts-> loading / saving themesreact-common\components\theming\ThemePickerModal.tsx& related elementsHow It Works
In short, when a user changes themes, we swap out the variable definitions and apply any override css directly on a
theme-overridestyle element in the document head (we create it if it does not exist). See theThemeManagerfor implementation.A Few Shortcuts I Took
Additional Notes
react-common\styles\semantic-ui-overrides.lessfile. I think this was still less work than removing semantic UI altogether. But it certainly isn't tidy. I didn't want to use!importantall over the place, so I've opted for using weirdly specific css selectors instead.Remaining Work (after PR)
Mirrored from upstream PR:
https://github.com/microsoft/pxt/pull/10391Created automatically by pr-sxs-human-evals for code-review agent comparison.
(URL wrapped in a code span so GitHub does not create a cross-reference on the upstream timeline.)
2 upstream conversation comments were not mirrored — see the upstream PR for full review context.