Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

feat(useSetDocumentColorScheme): set document color scheme css hook #2880

Merged
merged 8 commits into from Oct 6, 2022
Merged

feat(useSetDocumentColorScheme): set document color scheme css hook #2880

merged 8 commits into from Oct 6, 2022

Conversation

junwen-k
Copy link
Contributor

@junwen-k junwen-k commented Oct 5, 2022

Description

Introduced a react hook to set document color scheme css.

Related Issues

#2674

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run npm run lint:js -- --fix and/or npm run lint:md -- --fix for my JavaScript and/or Markdown changes.
    • This is important as most of the cases your code changes might not be correctly linted
  • I have run npm run test to check if all tests are passing, and/or npm run test -- -u to update snapshots if I created and/or updated React Components.
  • I have checked that the build works locally and that npm run build work fine.
  • I've covered new added functionality with unit tests if necessary.

@codecov-commenter
Copy link

codecov-commenter commented Oct 5, 2022

Codecov Report

Base: 66.02% // Head: 63.74% // Decreases project coverage by -2.27% ⚠️

Coverage data is based on head (ba66bed) compared to base (fd57b87).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2880      +/-   ##
==========================================
- Coverage   66.02%   63.74%   -2.28%     
==========================================
  Files         118      125       +7     
  Lines        1351     1462     +111     
  Branches      342      363      +21     
==========================================
+ Hits          892      932      +40     
- Misses        422      490      +68     
- Partials       37       40       +3     
Impacted Files Coverage Δ
src/__fixtures__/hooks.tsx 100.00% <ø> (ø)
src/__fixtures__/page.tsx 94.11% <ø> (-5.89%) ⬇️
...components/ApiComponents/Components/ApiHeading.tsx 0.00% <ø> (ø)
...rc/components/ApiComponents/Components/ApiLink.tsx 0.00% <ø> (ø)
...ents/ApiComponents/Components/SourceLink/index.tsx 0.00% <ø> (ø)
...ApiComponents/Components/VersionSelector/index.tsx 0.00% <ø> (ø)
src/components/ApiComponents/index.tsx 0.00% <ø> (ø)
src/components/Article/index.tsx 100.00% <ø> (ø)
src/components/Banner/index.tsx 100.00% <ø> (ø)
src/components/Codebox/index.tsx 100.00% <ø> (ø)
... and 83 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -0,0 +1,10 @@
import { useEffect } from 'react';

export function useSetDocumentColorScheme(theme: string | null): void {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I think this should be something like:

export function useSetDocumentColorScheme(): void {
  const [currentTheme, setCurrentTheme] = useState();

  useEffect(() => {
    // This is responsible for setting the color-scheme of the scroll-bars
    if (typeof document === 'object' && document.documentElement) {
      document.documentElement.style['color-scheme'] = theme;
    }
  }, [currentTheme]);

  return (theme: string | null) => setCurrentTheme(theme);
}

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

It looks okay-ish, but I would recommend approaching this differently. Also, please add missing unit tests 🙏

@junwen-k
Copy link
Contributor Author

junwen-k commented Oct 6, 2022

@ovflowd Thanks for the feedback :) I've updated the code to match the requirements. It just look a bit odd to have to keep track of two theme states though 👀

@junwen-k junwen-k requested a review from ovflowd October 6, 2022 08:36
@ovflowd
Copy link
Member

ovflowd commented Oct 6, 2022

It might sound odd because those two things are separate. One is the actual theme class and other is the color scheme 👀 but they should indeed be separated.

@ovflowd ovflowd merged commit 9a310c5 into nodejs:main Oct 6, 2022
@ovflowd ovflowd added the hacktoberfest-accepted Used to label PR's label Oct 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
hacktoberfest-accepted Used to label PR's
Projects
None yet
3 participants