Skip to content

Add optional themeOptions on FocusRingScope #11

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

Merged
merged 1 commit into from
Sep 14, 2022

Conversation

taingk
Copy link
Contributor

@taingk taingk commented Dec 2, 2021

Add light and dark css var and fix brightness calculation to allow these colors.
Brightness is a value from 0 to 1 and by default set to 0.2.
The prop themeOptions is optional and if undefined, Focus ring will have the same behavior as before. Otherwise, It will use light & dark theme (if saturation > 0.4).
Moreover, saturation could be part of themeOptions but I chose not to touch it.

Example :

<FocusRingScope containerRef={containerRef}>

No behavior change, it will still be primary or white ring

<FocusRingScope themeOptions={{ brightnessTreshold: 0.3 }} containerRef={containerRef}>

If the background brightness is 0.4, ring will be set to dark

<FocusRingScope themeOptions={{ brightnessTreshold: 0.3 }} containerRef={containerRef}>

If the background brightness is 0.2, ring will be set to light

Its an answer to this issue => #8

@Thisisjuke
Copy link

Hey @taingk ! Thx for your contribution and to try to fix our issue 🥺
I will review it soon and ask one of my colleague to do so !

@faultyserver can you give us your guidelines / insights to adapt this PR ?
Thx guys

@faultyserver
Copy link
Collaborator

Hi, sorry for the (absurdly) long time with no response here. This looks like a great solution to me, both for the configurability of the logic and the styling with --focus-light and --focus-dark. I think eventually it might be nice to have the brightnessThreshold be a global value that can be set rather than just per-scope, but I don't think that's a necessary addition for now.

I'm gonna go ahead and merge this in and get a new version out. Sorry again for the delay.

@faultyserver faultyserver merged commit 7a0ce37 into discord:main Sep 14, 2022
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