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

feat: matching border color for range selector in darkmode #1679

Merged
merged 3 commits into from
Sep 14, 2024

Conversation

zewa666
Copy link
Contributor

@zewa666 zewa666 commented Sep 13, 2024

I needed a new project to get to know the react wrapper of Slickgrid Universal better. So why not build a whole VSCode extension right ? ;)

cell-selector-darkmode

Also, I used that as the first time to try out the darkMode in more detail and noticed this little gotcha when selecting cells via mouse (at the end of the gif). This PR fixes it.

Copy link

stackblitz bot commented Sep 13, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

codecov bot commented Sep 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.8%. Comparing base (ac7e6f9) to head (53d0aa2).
Report is 7 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1679     +/-   ##
========================================
+ Coverage    99.8%   99.8%   +0.1%     
========================================
  Files         187     187             
  Lines       31087   31090      +3     
  Branches     9784    9786      +2     
========================================
+ Hits        30998   31001      +3     
  Misses         89      89             

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zewa666
Copy link
Contributor Author

zewa666 commented Sep 13, 2024

ouch ... turns out this ones much harder than thought since the grid isn't yet available in the ctor. The ! was misleading me to believe its already initiated.

Hm I could recreate the SlickCellRangeSelector inside init, or expose a public selectionCss override so we can keep the instance.

@ghiscoding
Copy link
Owner

ghiscoding commented Sep 13, 2024

Wow this is quite interesting, there's so much we can do with custom extensions in VSCode nowadays Does the filtering/sorting and everything else works like it does in a regular webpage? that's really something 😮

ouch ... turns out this ones much harder than thought since the grid isn't yet available in the ctor. The ! was misleading me to believe its already initiated.

ahh yeah that's my fault, the thing is that I often use this pattern because of these plugins have no choice but to execute the init() method, so technically speaking, the grid is already defined except in the constructor. If I make it optional then it adds optional chaining everywhere in the code even though we know that every plugin must be initialized no matter what. So I kinda went with ! because of that assumption but like you said, it's not available in the constructor. The thing too is that adding optional chaining everywhere will increase the code size, maybe less nowadays with newer browser supporting optional chaining by default but still (and I'm targeting--target es2021 for both ESM & CJS so maybe it's ok)

Hm I could recreate the SlickCellRangeSelector inside init, or expose a public selectionCss override so we can keep the instance.

it seems to work, so why not!
Just let me know when you're done with the PR, it looks fine from what I can see and if you want to refactor things around ! then feel free (though like I said, I did this in most of the plugins, that is the assumption of every plugin must be initialized)

@zewa666
Copy link
Contributor Author

zewa666 commented Sep 13, 2024

yeah seams so at least, hadnt had any issues so far.

I'll take one more look at it tomorrow and let you know when rdy to merge. perhaps I can find more stuff while at it

@ghiscoding
Copy link
Owner

I'll go ahead and merge, just because I'm going ahead with a new release. Thanks

@ghiscoding ghiscoding merged commit a15269e into ghiscoding:master Sep 14, 2024
6 checks passed
@zewa666
Copy link
Contributor Author

zewa666 commented Sep 14, 2024

one thing I havent checked but suspect is still missing is to react to option changes. but nevermind that can also be another PR

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