Skip to content
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

Open
wants to merge 1,811 commits into
base: release/job-posting-flow
Choose a base branch
from

Conversation

hansac91
Copy link
Collaborator

No description provided.

filwas and others added 30 commits September 5, 2024 19:54
…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
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
release: linkedin url bugfix
@@ -1,5 +1,5 @@
.container {
background: #04080d;
background-color: var(--bg-pm-color);

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';

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 {

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}

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.

Copy link

@NerdbordMentor NerdbordMentor left a 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.

Copy link
Collaborator

@Wiecek-K Wiecek-K left a 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)
    image
    image

  • on page "/profiles" (when you are unlogged) - there's one component without a light theme
    image

  • One icon on the landing page is missing color (in both themes)
    image

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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should put those styles into global mixins? and use it like this:
// Define the mixin for Body2 styles
@mixin body2 {
font-size: 14px;
font-weight: 400;
line-height: 20px;
margin-bottom: 24px;
}
// Include the mixin in a class
.my-class {
@include body2;
}

what do you think guys?

<FAQSection />
</Container>
<LandingFooter />
<ThemeWrapper>
Copy link
Collaborator

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>
Copy link
Collaborator

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);
Copy link
Collaborator

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!

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.

10 participants