feat: store color theme in state #8753

aoneill01 · 2023-02-10T16:30:32Z

Note that this is not merging into develop. Instead we will likely have a long-running feature branch for the color theme epic.

Resolves

Resolves ENA-236

Proposed Changes

Add a theme name to the Redux state
Create a framework for converting a theme name to a set of colors
Pass the colors from the selected theme to scratch-blocks

Reason for Changes

This is a first step towards the larger epic of creating high contrast and dark mode color themes for the editor. Eventually the user will be able to choose the color theme through the UI and see all of the editor components correctly themed. This change keeps the selected theme name in memory and converts it to a set of colors that will later be used to change styling.

Test Coverage

Added unit tests for logic that maps a theme name to a set of colors
Manual testing. Since there is not a user interface for changing themes yet, developer tools can be used to change the Redux state. I recommend using the Redux extension and dispatching the following payloads. (Or change the default initial theme in scratch-gui/src/reducers/theme.js.)

{
  type: 'scratch-gui/theme/SET_THEME',
  theme: 'high-contrast'
}

{
  type: 'scratch-gui/theme/SET_THEME',
  theme: 'standard'
}

{
  type: 'scratch-gui/theme/SET_THEME',
  theme: 'dark-mode'
}

Browser Coverage

Check the OS/browser combinations tested (At least 2)

Mac

Chrome
Firefox
Safari

Windows

Chrome
Firefox
Edge

Chromebook

Chrome

iPad

Safari

Android Tablet

Chrome

aoneill01 · 2023-02-10T16:48:28Z

src/lib/themes/index.js

+const themeMap = {
+    'dark-mode': mergeWithDefaults(darkMode),
+    'high-contrast': mergeWithDefaults(highContrast),
+    'standard': defaultColors
+};


I'm open to better names for our themes (ex standard / original / default / ???). This is easy to modify later too.

Right now the color data structure match the payload that is passed to scratch-blocks. I could see this being augmented later if we have some properties that are part of the editor but not in the blocks.

I think these names are fine for now, until/unless we settle on different user-facing names later.

I could see this being augmented later if we have some properties that are part of the editor but not in the blocks.

Makes sense! Seems like it would be pretty easy to turn it into theme = { blocksColors: {}, otherStuff: {} } if necessary.

ilikecereal1legacy · 2023-02-13T03:08:01Z

Tested on iPad Safari!
Dark mode:

High contrast:

Standard:

cwillisf

Looking good so far! I have some questions, but as far as overall structure & strategy, this makes sense to me.

cwillisf · 2023-02-13T15:40:02Z

src/containers/blocks.jsx

-    colours: {
-        workspace: '#F9F9F9',
-        flyout: '#F9F9F9',
-        toolbox: '#FFFFFF',
-        toolboxSelected: '#E9EEF2',
-        scrollbar: '#CECDCE',
-        scrollbarHover: '#CECDCE',
-        insertionMarker: '#000000',
-        insertionMarkerOpacity: 0.2,
-        fieldShadow: 'rgba(255, 255, 255, 0.3)',
-        dragShadowOpacity: 0.6
-    },


Does removing this mean there should be a .isRequired on Blocks.propTypes.options.colours?

Good point... Technically if it is left off the default scratch-blocks colors are used, which is nice. But make-toolbox-xml now expects certain colors to be provided. I've gone a different direction that I think accomplishes the same goal. I've passed the theme name to <Blocks />, including a default if it is not provided. This should ensure all of the colors the editor uses will be available to this component. See the latest commit.

cwillisf · 2023-02-13T15:45:16Z

src/lib/themes/index.js

+const themeMap = {
+    'dark-mode': mergeWithDefaults(darkMode),
+    'high-contrast': mergeWithDefaults(highContrast),
+    'standard': defaultColors
+};


I think these names are fine for now, until/unless we settle on different user-facing names later.

I could see this being augmented later if we have some properties that are part of the editor but not in the blocks.

Makes sense! Seems like it would be pretty easy to turn it into theme = { blocksColors: {}, otherStuff: {} } if necessary.

cwillisf · 2023-02-13T15:48:17Z

src/lib/make-toolbox-xml.js

    const stageSelected = ScratchBlocks.ScratchMsgs.translate(
        'MOTION_STAGE_SELECTED',
        'Stage selected: no motion blocks'
    );
    return `
-    <category name="%{BKY_CATEGORY_MOTION}" id="motion" colour="#4C97FF" secondaryColour="#3373CC">
+    <category name="%{BKY_CATEGORY_MOTION}" id="motion" colour="${colors.primary}" secondaryColour="${colors.tertiary}">


Is colors.tertiary always used to fill in a "secondary" color?

For the category styling, yes.

These aren't all of the uses, but for the block colors, scratch-blocks uses:

primary for the block background color

secondary for the dropdown background color

tertiary for the border color

These categories only have two colors for the circle icon: background (primary) and border (secondary). It is a bit unfortunate that the block's tertiary matches up to the category's secondary. Any suggestions to make this less confusing?

Gotcha, that makes sense. I probably knew that at some point in the past...

I think it could make sense to add a comment about this if you see a good place for it, but once it's established it'll go from "hmm, I wonder if this is a mistake" to "hmm, it's a bit odd that it worked out this way" -- less of a concern :)

👍 Added some comments to the code.

Racecar-Roaster · 2023-03-01T23:38:16Z

How does the mergewithdeafault work in a way it can process the dark mode code?

aoneill01 · 2023-03-02T14:05:59Z

mergeWithDefaults allows you to define just the colors that are different from the default theme, using defaultsDeep. My assumption is that some themes will share a lot of settings with the default theme. We'll see if that holds true. 🤞

Racecar-Roaster · 2023-03-02T22:08:20Z

Alright tak!

…

On Thu, Mar 2, 2023, 9:06 AM aoneill01 ***@***.***> wrote: mergeWithDefaults allows you to define just the colors that are different from the default theme, using defaultsDeep <https://lodash.com/docs/4.17.15#defaultsDeep>. My assumption is that some themes will share a lot of settings with the default theme. We'll see if that holds true. 🤞 — Reply to this email directly, view it on GitHub <#8753 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A3IHPMGGOWUZ2FNRSJ5BJ7TW2CSNFANCNFSM6AAAAAAUX7WHLU> . You are receiving this because you commented.Message ID: ***@***.***>

scratch-deployer · 2023-05-09T18:53:21Z

🎉 This PR is included in version 2.0.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

scratch-deployer · 2023-06-20T17:36:47Z

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

PaperMarioFan-2022 · 2024-05-14T16:12:55Z

I think this is will be a great feature!

aoneill01 commented Feb 10, 2023

View reviewed changes

apple502j mentioned this pull request Feb 12, 2023

Meta: Vanilla editor dark mode support ScratchAddons/ScratchAddons#5721

Open

aoneill01 requested a review from cwillisf February 13, 2023 15:18

cwillisf reviewed Feb 13, 2023

View reviewed changes

aoneill01 marked this pull request as ready for review February 15, 2023 13:58

feat: store color theme in state

b2d4021

aoneill01 force-pushed the feature/color-themes branch from 4b4db60 to b2d4021 Compare February 15, 2023 18:28

aoneill01 merged commit 225910d into scratchfoundation:feature/color-contrast Feb 15, 2023

aoneill01 deleted the feature/color-themes branch February 15, 2023 18:37

scratch-deployer added the released on @beta label May 9, 2023

scratch-deployer added the released label Jun 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: store color theme in state #8753

feat: store color theme in state #8753

aoneill01 commented Feb 10, 2023 •

edited

aoneill01 Feb 10, 2023

cwillisf Feb 13, 2023

ilikecereal1legacy commented Feb 13, 2023

cwillisf left a comment

cwillisf Feb 13, 2023

aoneill01 Feb 14, 2023

cwillisf Feb 13, 2023

cwillisf Feb 13, 2023

aoneill01 Feb 13, 2023

cwillisf Feb 14, 2023

aoneill01 Feb 14, 2023

Racecar-Roaster commented Mar 1, 2023

aoneill01 commented Mar 2, 2023

Racecar-Roaster commented Mar 2, 2023 via email

scratch-deployer commented May 9, 2023

scratch-deployer commented Jun 20, 2023

PaperMarioFan-2022 commented May 14, 2024

feat: store color theme in state #8753

feat: store color theme in state #8753

Conversation

aoneill01 commented Feb 10, 2023 • edited

Resolves

Proposed Changes

Reason for Changes

Test Coverage

Browser Coverage

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilikecereal1legacy commented Feb 13, 2023

cwillisf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Racecar-Roaster commented Mar 1, 2023

aoneill01 commented Mar 2, 2023

Racecar-Roaster commented Mar 2, 2023 via email

scratch-deployer commented May 9, 2023

scratch-deployer commented Jun 20, 2023

PaperMarioFan-2022 commented May 14, 2024

aoneill01 commented Feb 10, 2023 •

edited