Skip to content

Teacher Tool Theme Fixes #10481

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

Merged
merged 7 commits into from
Apr 3, 2025
Merged

Teacher Tool Theme Fixes #10481

merged 7 commits into from
Apr 3, 2025

Conversation

thsparks
Copy link
Contributor

@thsparks thsparks commented Apr 2, 2025

This fixes some issues with the teacher tool UI after themes were introduced. While doing this, I decided to fully swap out the themepack with our new theme variables, just to keep things consistent across the apps. It should also make things "just work" if we introduce other themes for teacher tool (though I haven't done that in this change).

Since I didn't want to introduce new theme colors, I have had to make some minor adjustments to how things look (like the beta tag & toasts), but I tried to stay relatively true to the original designs.

Upload Target: https://makecode.microbit.org/app/2c6d961517b971a19375a8090903860676c8e0f7-fa4ad57a6d--eval

@eanders-ms
Copy link
Collaborator

I think we should introduce bespoke color vars for toast now, and plan to move the Toast component into react-common "soon" (perhaps as part of the current microbit work).

@eanders-ms
Copy link
Collaborator

(minor) I think the sign in button lost its right margin:
image

@eanders-ms
Copy link
Collaborator

I think we should introduce bespoke color vars for toast now, and plan to move the Toast component into react-common "soon" (perhaps as part of the current microbit work).

but it's something we could certainly do later!

eanders-ms
eanders-ms previously approved these changes Apr 3, 2025
@thsparks
Copy link
Contributor Author

thsparks commented Apr 3, 2025

I think we should introduce bespoke color vars for toast now, and plan to move the Toast component into react-common "soon" (perhaps as part of the current microbit work).

Had a quick chat offline: concern here was around differentiation between color vars (red/yellow/green) and status (error/warning/success). We should probably have different theme colors for both, even if they sometimes duplicate each other. For example, micro:bit green is quite dark. It'd be nice for the success color to be a bit brighter. But I will do that in a separate change.

@thsparks thsparks merged commit d11bb91 into master Apr 3, 2025
6 checks passed
@thsparks thsparks deleted the thsparks/teacher_tool_theme_fixes branch April 3, 2025 21:16
thsparks added a commit that referenced this pull request Apr 4, 2025
I was originally thinking we could just use the colors-green, colors-yellow, and colors-red variables for this, but as was noted in #10481, those may be a bit too generic to work well for these scenarios. In the teacher tool, for example, the microbit green color is a little too dark to work well as a "success" color. Separating them will lead to some duplication in certain themes, but it's nice to have the flexibility.

As is the case for all theme colors, any themes that do not specify a value in their json file will default to the value in base-theme.less.

For now, I've only used these new colors in the teacher tool. We can swap them out in other areas as needed.
thsparks added a commit to microsoft/pxt-microbit that referenced this pull request Apr 4, 2025
In microsoft/pxt#10481, I have swapped out the themepack with our new theme variables, so we can remove the themepack file from microbit. The one thing we needed to keep was the font variable, so I've moved that into style.less instead.

I've also added a color that was missed in the original theme file. This already existed in arcade themes (and the base theme).
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.

2 participants