feat: add dark mode #2322

kaf-lamed-beyt · 2025-01-30T15:07:28Z

Hi @lidel, This is what I have so far. I'm currently using an approach involving a Context Provider to access the theme value across the entire application.

I'm also adding the styles individually in index.css by targeting the data-theme attribute. This is nice, but it can be quite tasking, hence my move towards a custom context hook. This would allow us to dynamically change colors across components in the application.

I ran into a slight blocker though. App.js uses a legacy React pattern — a class-based component which makes it a bit difficult to use the hook in this component, so I resorted to using the value from localStorage which is outside of the component/app life-cycle. So even if the value of theme in localStorage changes, the color — based on the ternary operation below — of the header won't change, because we do not trigger a re-render. Which is what the hook helps us achieve.

<div
     className="flex items-center ph3 ph4-l"
      style={{
         WebkitAppRegion: "drag",
          height: 75,
          background: currentTheme === "dark" ? "red" : "#F0F6FA",
          paddingTop: "20px",
          paddingBottom: "15px",
        }}
>

So, my question now is if it is possible to refactor App.js into a function-based component

welcome · 2025-01-30T15:07:32Z

Thank you for submitting this PR!
A maintainer will be here shortly to review it.
We are super grateful, but we are also overloaded! Help us by making sure that:

The context for this PR is clear, with relevant discussion, decisions
and stakeholders linked/mentioned.
Your contribution itself is clear (code comments, self-review for the
rest) and in its best form. Follow the code contribution
guidelines
if they apply.

Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
Next steps:

A maintainer will triage and assign priority to this PR, commenting on
any missing things and potentially assigning a reviewer for high
priority items.
The PR gets reviews, discussed and approvals as needed.
The PR is merged by maintainers when it has been approved and comments addressed.

We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution.
We are very grateful for your contribution!

kaf-lamed-beyt · 2025-02-01T06:30:54Z

Hi @lidel, kindly take a look at this when you get the chance. Thanks.

SgtPooki · 2025-02-05T16:23:00Z

So, my question now is if it is possible to refactor App.js into a function-based component

@kaf-lamed-beyt I would love for all components in ipfs-webui to be functional components. That is the direction we're moving. However, since that will likely be a significant change, we should probably do that as a separate PR.

You can see an example of a simple function component that is also using redux-bundler-react at

ipfs-webui/src/explore/explore-page-renderer.jsx

Line 32 in 7fbb51b

export default connect(

kaf-lamed-beyt · 2025-02-05T16:31:57Z

Oh! Thanks for the feedback, @SgtPooki! I'll take a look at the file you pointed me to now.

So, from the looks of things now, are you suggesting that i use the manual approach of targeting each element based on the styles in [data-theme="dark|light"] in index.css?

We can always keep the hook.

lidel · 2025-02-05T21:22:22Z

src/navigation/NavBar.js

+        <div className='mb4'>
+          <ThemeToggle />
+        </div>


💭 the space in the left NavBar is limited especially in ipfs-desktop where UI acts as native app. and window height is limited by default.

we want to add diagnostics screen at some point

perhaps it is better to move this toggle to top right corner where we already have two items:

thoughts?

oh, yeah! that sounds goood, @lidel!

i originally thought of placing it there, but couldn't locate the icons in the code. I'll try looking for them and update the theme toggle's position

SgtPooki

some comments

src/index.css

SgtPooki · 2025-02-11T19:09:56Z

src/context/theme-provider.tsx

+  const [theme, setTheme] = React.useState<boolean>(() => {
+    const savedTheme =
+      typeof window !== 'undefined' && localStorage.getItem('theme')
+    if (savedTheme) return savedTheme === 'dark'
+    return window.matchMedia('prefers-color-scheme: dark').matches
+  })
+  React.useEffect(() => {
+    const htmlElem = document.documentElement
+    const currentTheme = theme ? 'dark' : 'light'


seems like we can use better names for theme and setTheme since it seems to be a boolean indicating whether dark theme is selected or not

true. how does isDarkTheme and setDarkTheme sound?

src/hooks/theme.ts

SgtPooki · 2025-02-11T19:14:54Z

src/context/theme-provider.tsx

+    }
+  }
+  const values: ThemeContextValues = {
+    currentTheme: theme as unknown as Theme,


theme is boolean, not a Theme, so setting it to as unknown as Theme is wrong.

ideally, use of as (typescript casting) should be limited as much as possible.

thank you for catching this. I'll update it.

SgtPooki · 2025-02-11T19:18:16Z

src/context/theme-provider.tsx

+export interface ThemeProviderProps {
+  children: React.ReactNode
+}


instead of explicitly setting children for props, you can extend React.FC<PropsWithChildren> or similar..

or, my preference since there are no custom props, remove this and do it below

Suggested change

export interface ThemeProviderProps {

children: React.ReactNode

}

I think there's this minor issue with using React.FC in newer versions of React and I understand that it sits well with this component since it expects children which React.FC<PropsWithChildren> provides implicitly.

why shouldn't we set the children prop explicitly?

@kaf-lamed-beyt no huge reason, but here are a few:

prevent issues if React changes the type of the children prop

less code to maintain

using prop types from react directly prevents needing to add other props if we need to extend them (adding className or other common props in the future)

I think there's this minor issue with using React.FC in newer versions of React

I haven't ran into this, can you link to examples?

huh. makes sense.

for this 👇🏼

using prop types from react directly prevents needing to add other props if we need to extend them (adding className or other common props in the future)

I may be wrong... but, can't we do ...props?

I haven't ran into this, can you link to examples?

This issue from facebook/create-react-app sheds more light on why they deprecated React.FC @SgtPooki

SgtPooki · 2025-02-11T19:18:27Z

src/context/theme-provider.tsx

+const createThemeContext = () => React.createContext<ThemeContextValues | null>(null)
+export const ThemeContext = createThemeContext()
+
+export const ThemeProvider = ({ children }: ThemeProviderProps) => {


Suggested change

export const ThemeProvider = ({ children }: ThemeProviderProps) => {

export const ThemeProvider: React.FC<PropsWithChildren> = ({ children }) => {

src/context/theme-provider.tsx

SgtPooki · 2025-02-11T19:20:46Z

src/context/theme-provider.tsx

+  toggleTheme: () => void;
+  toggleThemeWithKey: (event: React.KeyboardEvent<HTMLButtonElement>) => void;


I'm not convinced we need both of these. Can toggleTheme accept an optional event and handle expected keys?

i'll try refactoring this.

Co-authored-by: Russell Dempsey <[email protected]>

kaf-lamed-beyt · 2025-02-11T23:43:51Z

@lidel, I've moved the toggle from the navbar. Here's what it looks like now.

I still have to wrap up with the other routes. When I do, I'll push the changes.

cc: @SgtPooki

- previously, pretty much all the keys on the keyboard can trigger the theme toggle when it is focused. we don't want that. Instead we should only limit it to specific keys (spacebar & enter). - include all the varying colors when the theme is dark on all routes - extend the Box component to use the theme value instead of explicitly passing it in the style prop across the codebase

kaf-lamed-beyt · 2025-02-12T15:51:15Z

done with this. @SgtPooki, @lidel, kindly review.

the explore route was quite tricky to work on. I couldn't locate the elements/components that are used to render data in the UI, that's why the one on the right still retained a white background.

perhaps, if anyone can point me to where they are exactly.

feat: add theme provider

8fcc565

kaf-lamed-beyt requested a review from a team as a code owner January 30, 2025 15:07

kaf-lamed-beyt changed the title ~~feat: add theme provider~~ feat: add dark mode Jan 30, 2025

lidel reviewed Feb 5, 2025

View reviewed changes

SgtPooki reviewed Feb 11, 2025

View reviewed changes

kaf-lamed-beyt temporarily deployed to Deploy February 11, 2025 19:24 — with GitHub Actions Inactive

kaf-lamed-beyt and others added 8 commits February 11, 2025 21:06

Update src/index.css

e8d30c3

Co-authored-by: Russell Dempsey <[email protected]>

Update src/index.css

4a61e2a

Co-authored-by: Russell Dempsey <[email protected]>

Update src/index.css

09ec2ca

Co-authored-by: Russell Dempsey <[email protected]>

Update src/hooks/theme.ts

180dd54

Co-authored-by: Russell Dempsey <[email protected]>

Update src/context/theme-provider.tsx

80ae8f2

Co-authored-by: Russell Dempsey <[email protected]>

Update src/hooks/theme.ts

ea53fa6

Co-authored-by: Russell Dempsey <[email protected]>

Update src/hooks/theme.ts

86e3fee

Co-authored-by: Russell Dempsey <[email protected]>

refactor: useTheme hook based on feedback provided

f0c5ad4

kaf-lamed-beyt temporarily deployed to Deploy February 12, 2025 11:36 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add dark mode #2322

feat: add dark mode #2322

kaf-lamed-beyt commented Jan 30, 2025

welcome bot commented Jan 30, 2025

kaf-lamed-beyt commented Feb 1, 2025

SgtPooki commented Feb 5, 2025

kaf-lamed-beyt commented Feb 5, 2025

lidel Feb 5, 2025 •

edited

Loading

kaf-lamed-beyt Feb 6, 2025

SgtPooki left a comment

SgtPooki Feb 11, 2025

kaf-lamed-beyt Feb 11, 2025

SgtPooki Feb 11, 2025

SgtPooki Feb 11, 2025

SgtPooki Feb 11, 2025

kaf-lamed-beyt Feb 11, 2025

SgtPooki Feb 11, 2025

kaf-lamed-beyt Feb 11, 2025

kaf-lamed-beyt Feb 11, 2025

SgtPooki Feb 12, 2025

SgtPooki Feb 12, 2025

kaf-lamed-beyt Feb 12, 2025

SgtPooki Feb 11, 2025

SgtPooki Feb 11, 2025

kaf-lamed-beyt Feb 11, 2025

kaf-lamed-beyt commented Feb 11, 2025

kaf-lamed-beyt commented Feb 12, 2025

	export interface ThemeProviderProps {
	children: React.ReactNode
	}

	export const ThemeProvider = ({ children }: ThemeProviderProps) => {
	export const ThemeProvider: React.FC<PropsWithChildren> = ({ children }) => {

		toggleTheme: () => void;
		toggleThemeWithKey: (event: React.KeyboardEvent<HTMLButtonElement>) => void;

feat: add dark mode #2322

Are you sure you want to change the base?

feat: add dark mode #2322

Conversation

kaf-lamed-beyt commented Jan 30, 2025

welcome bot commented Jan 30, 2025

kaf-lamed-beyt commented Feb 1, 2025

SgtPooki commented Feb 5, 2025

kaf-lamed-beyt commented Feb 5, 2025

lidel Feb 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SgtPooki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaf-lamed-beyt commented Feb 11, 2025

kaf-lamed-beyt commented Feb 12, 2025

lidel Feb 5, 2025 •

edited

Loading