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,812 commits into
base: release/job-posting-flow
Choose a base branch
from

Conversation

hansac91
Copy link
Collaborator

No description provided.

mr-fox93 and others added 30 commits September 5, 2024 20:14
…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
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