Add Theme Toggle / Dark Mode #31
Conversation
Reviewer's Guide by SourceryThis pull request introduces a new Theme Toggle component to support dark mode in the application. The README.md file has been updated with detailed instructions on how to use the new component. The menu component has been modified to include a 'border-border' class for proper border color in dark mode, and corresponding tests have been updated. Additionally, the new ThemeToggle module has been added with implementation details and customization options. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @trance128 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a brief example of how to use the Theme Toggle component in the main README.md file for quick reference.
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟡 Documentation: 2 issues found
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| @@ -0,0 +1,186 @@ | |||
| defmodule SaladUI.ThemeToggle do | |||
There was a problem hiding this comment.
suggestion: Consider moving JavaScript code to a separate file
The JavaScript code for theme switching is currently included in the module documentation. Moving this to a separate .js file would improve maintainability and allow for easier testing.
| defmodule SaladUI.ThemeToggle do | |
| defmodule SaladUI.ThemeToggle do | |
| @moduledoc """ | |
| Implementation of Theme Toggle from https://ui.shadcn.com/docs/dark-mode/next | |
| """ | |
| @external_resource "assets/js/theme_toggle.js" | |
| end |
| attr :class, :string, default: "" | ||
| attr :on_theme_change, :any, default: "set_theme", doc: "A string (event name) or 1-arity function to handle theme changes. Omit to use default js functions" | ||
|
|
||
| def theme_toggle(assigns) do |
There was a problem hiding this comment.
suggestion: Consider breaking down the theme_toggle function
The theme_toggle function is quite long and handles multiple responsibilities. Breaking it down into smaller, more focused functions could improve readability and maintainability.
| def theme_toggle(assigns) do | |
| def theme_toggle(assigns) do | |
| ~H""" | |
| <div class={@class}> | |
| """ | |
| end | |
| def render_theme_toggle(assigns) do | |
| theme_toggle(assigns) | |
| end |
| on_theme_change.(theme) | ||
| end | ||
|
|
||
| defp sun_icon(assigns) do |
There was a problem hiding this comment.
suggestion: Consider moving icon functions to a separate module
If the sun_icon and moon_icon functions are used elsewhere in the application, consider moving them to a separate icons module for better organization and reusability.
| """ | ||
| end | ||
|
|
||
| defp theme_change_event(theme, on_theme_change) when is_binary(on_theme_change) do |
There was a problem hiding this comment.
suggestion: Add error handling for theme change events
Consider adding error handling for cases where on_theme_change is neither a string nor a function. This would improve the robustness of the component.
| defp theme_change_event(theme, on_theme_change) when is_binary(on_theme_change) do | |
| defp theme_change_event(theme, on_theme_change) when is_binary(on_theme_change) do | |
| JS.dispatch(on_theme_change, detail: %{theme: theme}) | |
| end | |
| defp theme_change_event(_theme, _on_theme_change) do | |
| {:error, "Invalid on_theme_change value"} | |
| end |
|
|
||
| 3. Implement theme switching functionality: You can either use the provided JavaScript or implement your own theme switching logic. | ||
|
|
||
| Using the Provided JavaScript |
There was a problem hiding this comment.
suggestion (documentation): Inconsistent title case.
The phrase 'Using the Provided JavaScript' should be in title case for consistency, i.e., 'Using the provided JavaScript'.
| Using the Provided JavaScript | |
| Using the provided JavaScript |
| }); | ||
| ``` | ||
|
|
||
| Using Custom Theme Switching Logic |
There was a problem hiding this comment.
suggestion (documentation): Inconsistent title case.
The phrase 'Using Custom Theme Switching Logic' should be in title case for consistency, i.e., 'Using custom theme switching logic'.
| Using Custom Theme Switching Logic | |
| Using custom theme switching logic |
Add shadcn's Theme Toggle and support for darkmode.
Also, menu has been updated with the border-border class, which is needed to use the correctly colored border in dark mode
Summary by Sourcery
This pull request adds a Theme Toggle component to enable dark mode support in Phoenix LiveView applications. It also updates the menu component for proper border coloring in dark mode and includes comprehensive documentation and tests for the new feature.