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

Settings scope, foreground color, and window focus activation #4

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

jlawrence124
Copy link

@jlawrence124 jlawrence124 commented Nov 4, 2023

Hey @victorgz !

I've added several new features! Let me know if you have any questions or concerns.

  1. Settings scope update - users can now select where settings are saved (user vs workspace settings)
    demo_settings

  2. Controllable foreground color - users can now (optionally) set foreground color for both activity bar and status bar

    Screenshot 2023-11-04 at 3 09 46 AM

    demo_workspace_option

    demo_without_activity_bar

  3. Settings are now recalculated when changing window focus
    https://github.com/victorgz/vscode-salesforce-colorg/assets/18340994/78b4783e-3bc1-41e1-8f0c-180dbc2c10f5

@victorgz
Copy link
Owner

victorgz commented Nov 7, 2023

Wow thanks a lot for the PR @jlawrence124 ! I am quite busy lately but I'll book some time in my calendar to have a look at this in detail this week. I'll keep you posted!

@jlawrence124
Copy link
Author

Sure thing, @victorgz ! Take your time, no rush. Also, feel free to reach out if you have any questions or concerns I can address.

Copy link
Owner

@victorgz victorgz left a comment

Choose a reason for hiding this comment

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

Hey @jlawrence124, first of all: a huge thanks and congratulations for this PR! Honestly I think you have done a great job contributing to this extension 🚀

I've just added some comments, let me know what you think 😄

Also, we will need to think about updating the documentation (README.md) to include the two new config options (scope & foreground color)

@jlawrence124
Copy link
Author

@victorgz

These are all great suggestions! I will get to them ASAP. Thanks again for being open to collaboration on this!

@victorgz
Copy link
Owner

victorgz commented Nov 11, 2023

@jlawrence124
Perfect, no rush!

@jlawrence124
Copy link
Author

ok i think i hit everything, but just let me know if I'm missing something. I am working on automatically removing all color settings if the user changes anything related to the scope (just makes things a bit more user friendly), but will work on that tomorrow.

@victorgz
Copy link
Owner

victorgz commented Dec 8, 2023

Hi @jlawrence124 , thanks again for you contribution, and sorry for the late response (I've been OoO and extremely busy).
I've done some tests and I get some inconsistencies when switching workspaces. My configuration is:

  • Settings Scope = user
  • I've emptied the workbench.colorCustomizations both workspaces and the user... so I start with a clean configuration

If you can see in the GIF, sometimes the configuration is not being loaded when switching from one workspace to another...
Issue-user-settings

I was wondering whether there's something asynchronous happening behind the scenes that we're missing? Any idea?

@jlawrence124
Copy link
Author

hey @victorgz ! not a problem! I just moved into a new house last week so i understand 😆 . As for this sticky issue here, I'm going to dive into it this week and see if i can figure out what's going on. Thanks for your patience, will make sure this bug gets squashed 🐛

@jlawrence124
Copy link
Author

jlawrence124 commented Dec 29, 2023

Hey @victorgz just getting back around to this now. Moving and the holidays have been a bit hectic. It looks like the issue was coming from the defocusing logic combined with the asynchronous call to check if the focused window is a SF project. Should be fixed and much more performant now!

Screen.Recording.2023-12-29.at.12.58.10.AM.mov

Also found a small bug and squashed it where the initial config would apply even if the window wasn't focused.
^ will get back to this, looks like it was causing some issues

If this is good, let me know and I'll draft up some changes to the README

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.

2 participants