-
Notifications
You must be signed in to change notification settings - Fork 928
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
Dark mode #4244
Dark mode #4244
Conversation
Mostly kind of works. The palette buttons should prob. not change to black on blue. The palette background should not be white. The block colors themselves should probably be adjusted, esp. if the label text is now white. |
Noted 👍 |
Hi @Commanderk3 , I find the working and implementation of this feature really intriguing. I’d love the opportunity to collaborate with you on it. |
Sure, contact me in Matrix |
Sure I'll do that would like to know about all the updates there too |
My work on dark mode so far..... |
@walterbender Sir please review the latest update. The colors for blocks are darker now and redability is much better. |
js/palette.js
Outdated
.replace(/stroke_color/g, platformColor.ruleColor) | ||
.replace(/fill_color/g, platformColor.background), | ||
.replace("background_fill_color", platformColor.paletteLabelBackground) | ||
.replace(/stroke_color/g, "#E2E2E2") |
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.
Should these values be defined in platformColor?
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.
Yes, irrespective of light or dark mode, the stroke color is same for both. And I also created paletteLabelBackgroung because of its unique dark theme color.
js/toolbar.js
Outdated
@@ -1074,7 +1086,7 @@ function renderNewProjectConfirmation() { | |||
const confirmationButton = document.createElement("a"); | |||
confirmationButton.id = "new-project"; | |||
confirmationButton.style.display = "inline-block"; | |||
confirmationButton.style.backgroundColor = "#2196F3"; | |||
confirmationButton.style.backgroundColor = "#0066FF"; |
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.
Should this be defined in platformColor?
Looks really nice. My only doubt, aside from the few comments above, is whether or not we could generalize this in some way, to facilitate the user in generating their own custom themes. (I am not thinking of adding a theme editing interface so much as making it clear in the code and in perhaps a README how one would go about doing it.) |
In Music Blocks, elements are colored mainly in two ways. One is internal CSS where colors are derived from platformstyle.js and the other is through external CSS files. Block colors, pie menu, canvas, etc. are in platformstyle.js |
@walterbender Should I edit the README about how a user can customize theme? |
I think, from a developer perspective, we should also add some common themes ourselves, like Solarized Dark, Solarized Light, Minimalist Theme, Neon Theme, Pastel Theme, Retro Theme, and Glass-morphism for visual appeal, as well as themes like High-Contrast Theme, Grayscale Theme, Protanopia/Deuteranopia-Friendly Theme, and Tritanopia-Friendly Theme for color-blind people. To achieve this, In the end, From a purely user perspective, providing a color picker option in the pie menu (and other relevant areas) and then the input value event can be logged into storage and then changed in userTheme to assign it to the platformColor object would be nice. It can be downloaded locally and shared in JSON format if needed. It has its shares of problem which need to issued first like how we will need two different modules to handle internal CSS and external CSS, or keep only one kind of styling in JS increasing the JavaScript overhead. We can do something simple like hard-coding the user's choice into the DOM, but to tackle text and background differently, we will need two different options in the pie menu. These are some issues which come to the top of my mind as of now. But with this feature, we can also apply a gradient theme and a colored glassmorphism effect. This approach ensures modularity, making it easier for other developers to add and manage themes effectively. @walterbender and @Commanderk3, please share your thoughts on this. |
@arjunjayan999 |
I think a readme would be very useful to someone trying to develop their own theme. I don't think we want to go overboard with too many new themes. But if there is a high-contrast theme? I don't think we need Protanopia/Deuteranopia- and Tritanopia-friendly themes as there is built in value contrast as well as chromatic contrast in all of the palette color choices. |
If we do add more themes, we will prob. need to add a pie menu for theme selection. At that point, we'll want to explicitly add a custom theme. All of these ideas should come in a separate PR. |
Seems logical about the choice for color-blind options. A pie menu for theme selection and a custom theme option sound like valuable additions, after that it will be very easy to add custom themes which are approved of. I agree these changes should be handled in a separate PR to keep things organized. |
@walterbender I will make a commit for it. The README file will be beneficial. |
@walterbender I am a bit confused. Exactly where should I edit the README? |
Since most of the changes would be in platform.js, make a README.md file in js/utils. |
@walterbender README file added |
js/activity.js
Outdated
const confirmBtn = document.createElement("button"); | ||
confirmBtn.textContent = "Confirm"; | ||
confirmBtn.style.backgroundColor = "#2196F3"; | ||
confirmBtn.style.backgroundColor = "#0066FF"; |
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.
Still a few hardcoded color values. Any reason not to move these to platformColor?
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.
Irrespective of dark or light mode the color will be the same. But now we are talking about multiple themes so it makes sense to not hardcode them.
js/utils/README.md
Outdated
this.toggleDarkMode = () => { | ||
this.isDarkModeON = !this.isDarkModeON; // Toggle the boolean value | ||
try { | ||
this.storage.darkMode = this.isDarkModeON.toString(); // Save the state as a string |
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.
maybe this should be a theme name (so the same storage value can be used for any theme)?
Maybe we can land this and sort out the grid in a separate MR. |
There is, however, a conflict in Phrasemaker that needs to be sorted before I can merge. |
@walterbender What conflict sir ? |
@walterbender Sir why did the conflict occur? It usually does not happen. |
Something changed on the master branch since you made your PR branch that is incompatible with one of your changes. But it seems to be resolved. |
@walterbender I'll try and sort out the grid issue in the pr i will be creating which is going to add on onto the features diwangshu implemented in this pr. So basically this feature will be implemented in 2 prs ro completion. |
Dark Mode
I just want to make sure that whether this approach is good or not. Many of the colors in MB are derived from platformstyle.js
Modifying this file will require less hardcoding to change the theme to dark mode. When you toggle to dark mode the user has to reload the website to observe the changes. This is similar to how changing of languages works. With this PR, the isDarkModeON is stored locally (like beginnerMode). When the website starts it checks if isDarkModeON is true, if yes then it initializes a different set of colors (in platformstyle.js)
Of course there are some elements whose color is not defined by platformstyle.js . For them I guess we can define there color specifically.
@walterbender What do you think ?
The colors for dark mode can be better I guess.....