-
Notifications
You must be signed in to change notification settings - Fork 4
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/light dark mode implementation #380
base: release/job-posting-flow
Are you sure you want to change the base?
Feat/light dark mode implementation #380
Conversation
…ng-bug-with-clear fix: search input is sticky now also clear btn is stick under input
…ofile-by-hunter feat: notification sent to discord chanel
…input style: add rule for hide unnecessary icon on country input
…bile-view Fix/missing search input for mobile view
…l-fields-all-forms feat: adding info about optional fields in all forms
anchor button now ensures https
move ensureProtocol util from ui-system to platform-app
RELEASE: Bugfixes & improvements
SocialItems component uses <a href> instead of <AnchorButton>, so a separate fix is needed here
ensureProtocol in SocialItems
release: linkedin url bugfix
@@ -1,5 +1,5 @@ | |||
.container { | |||
background: #04080d; | |||
background-color: var(--bg-pm-color); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Using CSS variables for colors improves maintainability and theme support. Good job!
@@ -1,6 +1,6 @@ | |||
import { Dropbox } from 'dropbox'; | |||
import { readFile, unlink } from 'fs/promises'; | |||
import { basename } from 'path'; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: Good job removing the unused import statement. It keeps the code clean and maintainable.
import { LocaleSwitcher } from '../LocaleSwitcher/LocaleSwitcher' | ||
import { ThemeSwitcher } from '../ThemeSwitcher/ThemeSwitcher' | ||
import { ThemeWrapper } from '../ThemeSwitcher/ThemeWrapper' | ||
import styles from './Header.module.scss' | ||
|
||
interface HeaderProps { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: Great addition of the ThemeSwitcher
component. It enhances user experience by allowing theme toggling.
return ( | ||
<div className={`${styles['mode-slide-tab']}`}> | ||
<button | ||
onClick={toggleTheme} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Consider adding a transition effect when toggling themes for a smoother user experience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code changes effectively implement the light mode feature as required by the task. The user has added a theme switcher component and a theme wrapper to manage the theme state and persistence, which aligns with the requirement to add a switch for toggling between light mode and the existing mode. The use of CSS variables for styling adjustments across multiple components ensures consistency and responsiveness, which is a significant strength of this implementation. The addition of a floating theme switcher icon enhances the user interface, making it more intuitive for users to switch themes. The code changes also include the removal of an unused import in the Dropbox service, which is a good practice for maintaining clean code. Overall, the solution meets the task requirements and demonstrates a well-structured approach to implementing the light mode feature.
… into feat/light-dark-mode-implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good, I left a few tiny suggestions.
However, I noticed a few issues on the working page
-
contrast problems on the light theme (especially on the landing page)
-
on page "/profiles" (when you are unlogged) - there's one component without a light theme
-
One icon on the landing page is missing color (in both themes)
By the way, is it possible to add transition: background-color to every component?
//styleName: Header 2; | ||
font-size: 32px; | ||
font-weight: 400; | ||
line-height: 40px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<FAQSection /> | ||
</Container> | ||
<LandingFooter /> | ||
<ThemeWrapper> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the layout would be a better place for the Wrapper.
<h1 className={styles.title}>Good Dev Hunting</h1> | ||
</Link> | ||
<GitHubStarsButton /> | ||
<ThemeWrapper> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the theme wrapper in the page/layout will eliminate the need for it in this component
@@ -79,7 +79,7 @@ | |||
} | |||
|
|||
.name { | |||
color: #e2eaf1; | |||
color: var(--text-color); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using CSS variables for colors improves maintainability and theme support. Good job!
No description provided.