Add theme support#118
Conversation
There was a problem hiding this comment.
Hi Theiss,
Well documented code and has been well written.
I tested the code in firefox and it worked without error :)
Also I would consider just mentioning this request to Cameron as he would probably like to document this.
I approve this request :)
|
I've made the requested changes on themes.js. |
|
DescriptionI have updated the source themes.js code so it can be used in the index.html code with a selection of pre-defined themes. It adds an event listener for themeSelection so it can apply the theme that was selected. This image shows an example theme selected (Space):
|
cameronhum
left a comment
cameronhum
left a comment
There was a problem hiding this comment.
Hey Theiss,
Overall this is well done and is made with the future of the application in mind. The comments are well done and descriptive with that one tiny nitpick I mentioned. The applyTheme function does exactly as advertised in Chrome, Chromium based browsers, Edge and Firefox with no issues.
You did however add a new addition (theme selector) which I love the concept of and pushes this code into a user based use case, however it needs some touching up which I mentioned in earlier comments.
Once the changes are added I am happy to approve this.
I would also like to add the problems related to the themes are mainly relevant to the light theme. As such since this pull request main feature isn't the dropdown, if the light mode is removed rather than fixed and you are able to go back to default theme, that will also be acceptable for me to approve the review since the other themes all are functional and can be built upon in future pull requests.
|
I've made the changes to add the option to select the default theme and made sure that all the relevant code comments were there. I added an additional comment explaining that elements from the colours.css file can be added to the themes. |
|
I didn't change the GutterColour (main background column colour) but left it as a comment so future decision makers can add them if needed for each theme. Made sure that the 'yellow' editor strings (previously semi-concealed) were adjusted to a better colour for the light theme. Hopefully this should be sufficient enough for approval. Sincerely |
cameronhum
left a comment
cameronhum
left a comment
There was a problem hiding this comment.
Hey Theiss,
Other than the small correction which stops it from running at the moment. I manually fixed it and tested it and am happy with it now!
I will approve it as soon as that is fixed. If possible message me on teams so I am able to do this for you ASAP
| if (!sel) return; | ||
| sel.add(new Option("default / none", "")); //Blank value = go back to default | ||
| //Add every theme as an option | ||
| Object.keys(themes).forEach(name => sel.add(new Option(name, name))); /Visible text, value |
There was a problem hiding this comment.
'/Visible text, value'
this part causes an issue. needs '//'
cameronhum
left a comment
cameronhum
left a comment
There was a problem hiding this comment.
I have pulled the new changes and you have addressed the syntax error and I can confirm everything works as intended. I am happy to approve it now good job the functionality looks great!
WhyPenguins
left a comment
WhyPenguins
left a comment
There was a problem hiding this comment.
Hey all, just wanted to leave some thoughts 😃 The feature is really neat and I think looks good from a code perspective! I agree that the user facing part still needs some work though - either that or it could just be hidden for now.
A couple of the styles make the code/comments pretty hard to read:
For light mode I'd also expect something more like this I guess?
(just a very quick mockup based on https://github.com/nilshartmann/vscode-blue-light-theme/)
If you want to have a go at improving those feel free to, otherwise yeah might be best to just hide the selector for now until the styles are ready.
One other thing code wise, I think it'd be good to make the page's color-scheme also configurable (right now it's hard-coded in stylesheet.css -> body) - this'll be useful for making the scrollbars and such match in light mode 😄
|
All done now. Apologies for pushing changes to this pull request late. I couldn't manage to add any additional features so I just hid the themes tab instead. |
4 participants
Description
This update adds dynamic theming support to SplashKit Online by introducing a new applyTheme() function.
The function allows developers to override existing CSS variables at runtime using a JavaScript JSON object. It targets variables defined in /Browser_IDE/css/theming/colours.css and makes it easy to experiment with custom color schemes without editing the CSS file directly.
Required ./Javascript/themes.js inside the index.html
The function can also be tested in the browser console.