Skip to content
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

Create a test to make sure that editor theme is in sync with the app theme #49784

Open
Sboonny opened this issue Mar 21, 2023 · 14 comments · May be fixed by #54481
Open

Create a test to make sure that editor theme is in sync with the app theme #49784

Sboonny opened this issue Mar 21, 2023 · 14 comments · May be fixed by #54481
Labels
help wanted Open for all. You do not need permission to work on these. type: feature request Threads classified to be feature requests. Implementation to be considered as a nice to have

Comments

@Sboonny
Copy link
Member

Sboonny commented Mar 21, 2023

The test should account for this issue #48936, TL;DR of the issue, the editor had different theme than the one in the user preferred, and the test should make sure that the themes are always the same

Screen Shot 2023-01-03 at 3 12 59 PM

The test should account for two different state of the user, and 3 different states of the theme:

while the user is logged out:

  • visit /learn/2022/responsive-web-design/learn-html-by-building-a-cat-photo-app/step-23
  • set your preferred-scheme to light, themes should be in sync
  • set your preferred-scheme to dark, themes should be in sync

Note to set preferred-scheme from devtools press Ctrl + P, then press > and select it.

while the user is logged in

  • visit /learn/2022/responsive-web-design/learn-html-by-building-a-cat-photo-app/step-23
  • set your theme to default, themes should be in sync
  • set your theme to night, themes should be in sync

Additional context

Thanks to Tom link, you can find out how to test colors through Cypress through this article https://blog.devgenius.io/how-to-make-a-cypress-test-for-prefers-color-scheme-dark-42337138f727

Note: although I mentioned Cypress you are free to use any tools in the E2E folder

Here is the file: https://github.com/freeCodeCamp/freeCodeCamp/blob/main/client/src/templates/Challenges/classic/editor.tsx

Maybe I missed something, if you think there is more, please add it.

If you would like to fix this issue, please make sure you read our guidelines for contributing, we prioritize contributors following the instructions in our guides. Join us in our chat room or the forum if you need help contributing, our prolific contributors will guide you through this.

@Sboonny Sboonny added help wanted Open for all. You do not need permission to work on these. type: feature request Threads classified to be feature requests. Implementation to be considered as a nice to have labels Mar 21, 2023
@Fishbite
Copy link

Fishbite commented Mar 23, 2023

Ctrl + P is a Windows OS command to print the current document. Choose another key combination to avoid problems.

@ojeytonwilliams
Copy link
Contributor

It's chrome devtools that uses that shortcut.

@Fishbite
Copy link

Correct, not Firefox, MS Edge.... If the user is not specifically focused on the chrome devtools panel on a Windows m/c, it will pop-up the print dialogue .

@ojeytonwilliams
Copy link
Contributor

Right, but the context was setting the preferred theme inside devtools.

@Fishbite
Copy link

Fishbite commented Mar 23, 2023

Sure, and it is just a note after all, but chrome is not specifically mentioned and ctrl+p currently displays an open file dialogue... in chrome. I just think it would probably be wiser to pick another key-combo.

@Sboonny
Copy link
Member Author

Sboonny commented Mar 23, 2023

Please, let's not discuss something isn't progressing the issue forward, so far there hasn't been a discussion on how the test can be implemented.

If you are looking for ways to access the preferred-color-scheme in your browser, there are multiple guides online, and accessing the preferred-color-scheme is a tip not a requirement, the test can be written without open the dev-tools, although I wouldn't recommend that.

there is our chat room or the forum if you need help

@a2937
Copy link
Member

a2937 commented Mar 27, 2023

I believe the best spot for this to be implemented is editor.tsx.

My personal opinion is that the tests be grouped by light mode or dark mode since it can be stubbed before the visit. Though I am partial to the user signed in vs not signed in approach.

@a2937
Copy link
Member

a2937 commented Mar 28, 2023

I can't run onBeforeLoad before the window is ready because there will be no window object yet. I've tried using onLoad but the app isn't entering dark mode.

@Sboonny
Copy link
Member Author

Sboonny commented Mar 28, 2023

Hi @a2937, sorry for the delay, and confusing user stories.

The issue with logged-in user can happen as follows:

  • when the user is logged in and picked default (light) theme, and have the preferred color scheme to dark, the editor should follow the theme value and stay default (light)
  • When the user isn't logged in and have preferred-color-scheme set to dark, the editor should change to dark.

Edit: About the failing test, I don't know why it's failing, I can think of a reason why the class fails, because the editor use a map to add its styles, but not getting the editor is weird for me 🤔 😞

@a2937
Copy link
Member

a2937 commented Mar 29, 2023

I'm managed to make some progress. I finally have the dark theme show up. It appears that window.matchMedia was used more heavily than I thought. Now I just need to find the right class names for the editor palette and we should be golden.

@Sboonny
Copy link
Member Author

Sboonny commented Mar 30, 2023

That's the tricky part, there is no className for the editor, it uses a object to set its style, we are only adding values through defineTheme

const defineMonacoThemes = (
monaco: typeof monacoEditor,
options: { usesMultifileEditor: boolean }
) => {
if (monacoThemesDefined) {
return;
}
monacoThemesDefined = true;
const yellowColor = 'FFFF00';
const lightBlueColor = '9CDCFE';
const darkBlueColor = '00107E';
monaco.editor.defineTheme('vs-dark-custom', {
base: 'vs-dark',
inherit: true,
colors: {
'editor.background': '#2a2a40',
'editor.lineHighlightBorder': '#0e4470'
},
rules: [
{ token: 'delimiter.js', foreground: lightBlueColor },
{ token: 'delimiter.parenthesis.js', foreground: yellowColor },
{ token: 'delimiter.array.js', foreground: yellowColor },
{ token: 'delimiter.bracket.js', foreground: yellowColor }
]
});
monaco.editor.defineTheme('vs-custom', {
base: 'vs',
inherit: true,
// TODO: Use actual color from style-guide
colors: {
'editor.background': options.usesMultifileEditor ? '#eee' : '#fff',
'editor.lineHighlightBorder': '#cee8fc'
},
rules: [{ token: 'identifier.js', foreground: darkBlueColor }]
});
};

Here is the original file in monaco-editor https://github.com/microsoft/vscode/blob/93028e44ea7752bd53e2471051acbe6362e157e9/src/vs/editor/standalone/common/themes.ts

@a2937
Copy link
Member

a2937 commented Mar 31, 2023

Hang on. I'm still a little confused about something. I think I'm doing something wrong here or there's an error that I didn't see in the console. The editor itself isn't turning to dark-theme despite everything else is dark. Additionally while signed in on the local website instance I have running, it also appears fine.

@NoelCov
Copy link

NoelCov commented Jun 7, 2023

@a2937 Hey there, I was looking into this and was wondering if you're still working on it? I want to pick something up but not if someone is already working on it. Thanks! :)

@a2937
Copy link
Member

a2937 commented Jun 7, 2023

@NoelCov You are more than welcome to give this a try.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Open for all. You do not need permission to work on these. type: feature request Threads classified to be feature requests. Implementation to be considered as a nice to have
Projects
None yet
5 participants