-
Notifications
You must be signed in to change notification settings - Fork 113
feat: support cat-blocks as an optional theme #374
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
feat: support cat-blocks as an optional theme #374
Conversation
Test report for scratch-gui 2 files ±0 62 suites ±0 9m 35s ⏱️ -33s Results for commit a0dd45d. ± Comparison against base commit 9e87362. This pull request removes 15 and adds 15 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
adzhindzhi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great start! It does raise some UX questions, such as how we would toggle this on/off, where it would live in the UI, and whether we'd want to use it along with the high contrast theme (likely yes), etc. But that's not something we need to figure out now.
| // TODO: Make this depend on membership status | ||
| canChangeTheme: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think any change is necessary right now, but I can imagine a world where everyone can change themes, but some specific themes are exclusive. It might be that the theme menu should show up if .some(theme => theme.isAvailable(me)) or something like that. That could work if, for example, we still want cat blocks for everyone once a year.
| customProceduresVisible: state.scratchGui.customProcedures.active, | ||
| workspaceMetrics: state.scratchGui.workspaceMetrics, | ||
| useCatBlocks: isTimeTravel2020(state) | ||
| useCatBlocks: isTimeTravel2020(state) || state.scratchGui.settings.theme === CAT_BLOCKS_THEME |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| // Technically what we are persisting is the color mode, but for historical reasons, | ||
| // we should continue using 'scratchtheme' as the cookie key. | ||
| const COOKIE_KEY = 'scratchtheme'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✔️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to migrate this to a new key at some point, but I'd wait until we have a clearer picture of the future of themes and color modes first.
|
Looks like there's a legitimate test failure related to the settings menu. I played around with it locally, and the language & color mode menus weren't consistently closing each other as they should. I think it's related to clicking the submenus rather than hovering over them. |
Yeah, that was my bad, I seem to have been a bit overzealous in refactoring 😅. I've returned the corresponding redux actions ensuring only one sibling menu is open. |
Resolves
https://scratchfoundation.atlassian.net/browse/UEPR-416
Proposed Changes
Reason for Changes
As part of the membership initiative, we want to reintroduce the cat blocks theme as an optional theme for supporters of Scratch