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

Add themes support #874

Merged

Conversation

kewinzaq1
Copy link
Collaborator

@kewinzaq1 kewinzaq1 commented Dec 21, 2024

This pull request adds support for vscode themes
Connected (#729 (comment) , #866)

The changes add new theme based on vscode theme variables. As being aware that some themes may decrease readability, this PR adds option to use built-in themes. Built-in themes stay mostly not changed, only few additional variables that allow better customization with the new changes.

night-owl.mov
cursor-default.mov
vscode-default.mov
palenight.mov

Copy link

vercel bot commented Dec 21, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
radon-ide ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 14, 2025 5:03pm

@kewinzaq1 kewinzaq1 changed the title feat: preview background Add themes support Dec 21, 2024
Copy link
Collaborator

@filip131311 filip131311 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, thank you for your time.
In general I really like the changes unfortunately there are some parts of the application that lose on readability after the changes, for example lunchConfiguration:
Screenshot 2024-12-23 at 10 08 55

Vscode has styled inputs:
Screenshot 2024-12-23 at 10 11 28
Screenshot 2024-12-23 at 10 11 35

so i believe you can find the styles for them

I also left one inline question

Thank u again and merry Christmas

@kewinzaq1
Copy link
Collaborator Author

Hello, thank you for your time. In general I really like the changes unfortunately there are some parts of the application that lose on readability after the changes, for example lunchConfiguration: Screenshot 2024-12-23 at 10 08 55

Vscode has styled inputs: Screenshot 2024-12-23 at 10 11 28 Screenshot 2024-12-23 at 10 11 35

so i believe you can find the styles for them

I also left one inline question

Thank u again and merry Christmas

Thanks for the review merry Christmas 🎄

@kmagiera
Copy link
Member

kmagiera commented Jan 8, 2025

I'd also be in favor of supporting themes properly. The reason we decided to shift away from this approach initially was that it was difficult to control some of the theme parameters under different vscode settings. This resulted in a number of elements being difficult to use as they'd blend inproperly into elements in the background for example.

One way this can be solved is be giving an option whether the IDE should use the editor theme or the built-in one. I wonder how difficult would it be to incorporate that while still relying mostly on css variables.

@kewinzaq1
Copy link
Collaborator Author

One way this can be solved is be giving an option whether the IDE should use the editor theme or the built-in one. I wonder how difficult would it be to incorporate that while still relying mostly on css variables.

@kmagiera I think that it should go quite smoothly. I'll return with an example of it. One thing that is striking in this approach is that it would hurt UX a bit, forcing users to manually switch to the default theme. You know, I mean the experience that something does not look well and then someone would report this because overlooked this option in workspace config or so. But maybe I exaggerate.

@kmagiera
Copy link
Member

kmagiera commented Jan 9, 2025

I agree such a setting would be difficult to discover. Perhaps we should default to the editor theme and only expect the setting to be used as a fallback when the UI doesn't look good in certain theme

@kmagiera
Copy link
Member

This looks good. Thank you. Anything you're planning to change as this PR is still marked as draft? We won't be merging it for the upcoming release (this week), but may want to include it for the following one

@kewinzaq1
Copy link
Collaborator Author

This looks good. Thank you. Anything you're planning to change as this PR is still marked as draft? We won't be merging it for the upcoming release (this week), but may want to include it for the following one

@kmagiera great, I'm not planning to change anything for now, so I think we can remove the draft label

@kewinzaq1 kewinzaq1 marked this pull request as ready for review January 14, 2025 16:26
@kewinzaq1 kewinzaq1 requested a review from filip131311 January 14, 2025 16:31
@kewinzaq1 kewinzaq1 force-pushed the add-vscode-themes-support branch from 7c4cda2 to b34c682 Compare January 14, 2025 16:33
…onally add small horizontal margins in some views because input was cut off.

Tested views:
1. Manage devices view and rename devices
2. Launch config modal
3. Localization modal
4. Location modal
…View`

similarly to how it's done with `Input`
Copy link
Collaborator

@filip131311 filip131311 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome, thank you so much for your contribution!

We will include this in next release.

@filip131311 filip131311 merged commit 7d0457d into software-mansion:main Jan 20, 2025
4 checks passed
kmagiera added a commit that referenced this pull request Feb 7, 2025
This PR fixes issue with styles of debugger overlay in editor-match mode
after #874

Before:
<img width="465" alt="image"
src="https://github.com/user-attachments/assets/49767965-b2c3-4887-9d25-6ee67d537e9f"
/>

After:
<img width="470" alt="image"
src="https://github.com/user-attachments/assets/878cbb1f-1e15-49d9-b494-18520688fc37"
/>
<img width="467" alt="image"
src="https://github.com/user-attachments/assets/b42c75ee-3a03-4c16-975c-af78119a2c93"
/>

### How Has This Been Tested: 
1. Make the debugger pause and observe the styles
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants