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

fix: Support Dark theme based on the web browser's theme #62

Merged
merged 5 commits into from
Nov 30, 2023

Conversation

SankalpHaritash21
Copy link
Contributor

Summary

Adding Dark Theme feature in add blog, home page.

Description

In this project adding dark theme feature according to the browser theme.

Images

Add blog page
add

Home page

home

Details page

detail

Issue(s) Addressed

Prerequisites

Copy link

vercel bot commented Nov 23, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
wanderlust ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 27, 2023 3:58am
wanderlust-backend ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 27, 2023 3:58am

@SankalpHaritash21
Copy link
Contributor Author

SankalpHaritash21 commented Nov 23, 2023

Sir number of div.
And Sorry for the delay.
If any change needed plz let me know I will make that changes.
Sir In some places I used little bit different approach

frontend/tailwind.config.js Outdated Show resolved Hide resolved
Copy link
Owner

@krishnaacharyaa krishnaacharyaa left a comment

Choose a reason for hiding this comment

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

Thank you for the dark theme support @SankalpHaritash21 it would really help our project. Kindly address the review comments, we are almost good to merge

frontend/src/pages/add-blog.tsx Show resolved Hide resolved
frontend/src/pages/add-blog.tsx Show resolved Hide resolved
frontend/src/pages/details-page.tsx Outdated Show resolved Hide resolved
frontend/src/pages/details-page.tsx Outdated Show resolved Hide resolved
frontend/src/pages/home-page.tsx Show resolved Hide resolved
frontend/tailwind.config.js Outdated Show resolved Hide resolved
@krishnaacharyaa
Copy link
Owner

krishnaacharyaa commented Nov 26, 2023

image

Deployment looks like this to me, doesn't match with the description of the PR :)

@SankalpHaritash21
Copy link
Contributor Author

SankalpHaritash21 commented Nov 26, 2023

@krishnaacharyaa I have check in my system it is updated I thought I forget to upload components folder.

I uploaded all the files. Plz check preview.

@SankalpHaritash21
Copy link
Contributor Author

SankalpHaritash21 commented Nov 28, 2023

Hi, @krishnaacharyaa, @chinmaykunkikar plz review the code now it is working fine.

@krishnaacharyaa
Copy link
Owner

@SankalpHaritash21 Thanks for the update. Looks good to me, if you observe carefully the shimmer cards don't support the dark theme, Can you kindly raise a bugfix ticket, post that we will merge this :)

@SankalpHaritash21
Copy link
Contributor Author

Ok

@chinmaykunkikar
Copy link
Collaborator

@krishnaacharyaa are we keeping the dark theme by default or are we giving users an option to toggle?

@krishnaacharyaa
Copy link
Owner

@krishnaacharyaa It's possible using husky and pre-commit hooks. But, let's not do that. Let's not make it a pain in the bum for the devs who have worked on something and want to commit their work. We can explore some other solutions for what you're mentioning.

@chinmaykunkikar, just a confirmation, how does it causes trouble to the devs? I meant when someone tries to commit which doesn't obey the predefined regex we give some friendly error message saying "Use this regex for the commit message". If we can get this done using the husky, then I think we are good. Happy to consider your recommendations :)

@SankalpHaritash21
Copy link
Contributor Author

@krishnaacharyaa do we need any other changes or it look good

@krishnaacharyaa
Copy link
Owner

Please enter your GitHub URL to provide a reproduction of the issue

@SankalpHaritash21
Reproduction URL is the present deployment url,
Kindly give good reproduction steps, you need to say open url and then switch to the dark theme, then before the posts are loaded we see shimmer cards which are white.
Screenshot is not previewable, kindly update it

@SankalpHaritash21
Copy link
Contributor Author

@krishnaacharyaa I will update it

@SankalpHaritash21
Copy link
Contributor Author

@krishnaacharyaa what to add in screenshot

@chinmaykunkikar
Copy link
Collaborator

chinmaykunkikar commented Nov 28, 2023

@SankalpHaritash21
Reproduction URL is the present deployment url,
Kindly give good reproduction steps, you need to say open url and then switch to the dark theme, then before the posts are loaded we see shimmer cards which are white.
Screenshot is not previewable, kindly update it
@krishnaacharyaa I will update it
@krishnaacharyaa what to add in screenshot

Let's limit the discussion of #64 to #64 please 😛

@chinmaykunkikar
Copy link
Collaborator

@chinmaykunkikar, just a confirmation, how does it causes trouble to the devs? I meant when someone tries to commit which doesn't obey the predefined regex we give some friendly error message saying "Use this regex for the commit message". If we can get this done using the husky, then I think we are good. Happy to consider your recommendations :)

#65

@chinmaykunkikar
Copy link
Collaborator

@krishnaacharyaa @SankalpHaritash21 I have fixed the dark mode skeleton shimmer colors locally. I have 2 versions, one without background to individual cards, and another with the background.
Take a look.

  • v1
    v1 Fix dark shimmer of skeleton

  • v2
    v2 Fix dark shimmer of skeleton

What do you think? Should I add these changes to this this PR, and eliminate the need for #64?

@krishnaacharyaa
Copy link
Owner

krishnaacharyaa commented Nov 29, 2023

@chinmaykunkikar, thank you for the quick fix, just a small observation, v1 looks good to me as the color looks subtle and doesn't pop off, but the blog post card's colour seems mismatching the featured and latest cards, can you kindly confirm on that, because sometimes images are different than actual colours, And I'm fine with your final call, this was my preference and observation ;)

Can we do it in seperate PR, helps us to keep it seperate commit, I'll squash the present PRs commit because of the commit messages...

P.S: I guess it's the shimmer which is giving the different colours 😂, my bad, V2's padding looks cool to me, feel free to take your call 😊

@SankalpHaritash21
Copy link
Contributor Author

@chinmaykunkikar plz let me first complete this task then you can do these changes plz

@krishnaacharyaa
Copy link
Owner

@SankalpHaritash21, I guess it @chinmaykunkikar already has fix for #64 , So i guess you can pick up some other story :)

@SankalpHaritash21
Copy link
Contributor Author

@krishnaacharyaa I am saying that @chinmaykunkikar can push changes after completing this issue

@chinmaykunkikar
Copy link
Collaborator

chinmaykunkikar commented Nov 29, 2023

Can we do it in seperate PR, helps us to keep it seperate commit, I'll squash the present PRs commit because of the commit messages...

Okay. I'll make those changes on a separate branch and push it to #64 once this is merged.

@SankalpHaritash21 For the color, use slate-900 as dark-bg and slate-800 for the textfield variable.
You can introduce those variables in tailwind config file and remove the existing ones from index.css file.
Please refer to this documentation on how to use custom variable names: https://tailwindcss.com/docs/customizing-colors

Also, @SankalpHaritash21, don't work on anything related to skeletons/shimmer.

@chinmaykunkikar
Copy link
Collaborator

chinmaykunkikar commented Nov 29, 2023

P.S: I guess it's the shimmer which is giving the different colours 😂, my bad, V2's padding looks cool to me, feel free to take your call 😊

Correct 😆
I'll see what looks the best here after this pr is completed :)

@chinmaykunkikar
Copy link
Collaborator

@krishnaacharyaa @SankalpHaritash21

The only thing that's pending for this PR is the color variable names, right? What else?

I'll be taking up the skeleton ui task.

@krishnaacharyaa
Copy link
Owner

@krishnaacharyaa @SankalpHaritash21

The only thing that's pending for this PR is the color variable names, right? What else?

I'll be taking up the skeleton ui task.

Yes @chinmaykunkikar, you are right

@krishnaacharyaa
Copy link
Owner

@SankalpHaritash21, please follow the above comment and finish this, lets merge this PR, it's been really long

@SankalpHaritash21
Copy link
Contributor Author

#62 (comment)

@krishnaacharyaa I will try to complete it by today.

@SankalpHaritash21
Copy link
Contributor Author

@chinmaykunkikar @krishnaacharyaa I used this approach

/** @type {import('tailwindcss').Config} */

export default {
content: ['./index.html', './src/**/*.{js,ts,jsx,tsx}'],
darkMode: 'media',
theme: {
extend: {
colors: {
dark: {
DEFAULT: '#0f172a',//bg-slate-900
textfield: '#1e293b',//bg-slate-800
},
},
},
},
plugins: [],
};

and removed code from index.css.
Can u plz

11

@SankalpHaritash21
Copy link
Contributor Author

SankalpHaritash21 commented Nov 30, 2023

@krishnaacharyaa @chinmaykunkikar check then I will push the code. if this approach is ok or not

#62 (comment)
ok @chinmaykunkikar I will take some another task @

@krishnaacharyaa
Copy link
Owner

krishnaacharyaa commented Nov 30, 2023

@SankalpHaritash21 Looks good to me, but I see the tw-classes are commented out, if the image is of the commented classes then fine

@SankalpHaritash21
Copy link
Contributor Author

#62 (comment)

@krishnaacharyaa I tryed to use tw-classes names but it is breaking the code and I also read tw docs but I found this way is effective that's why I use hex code of slate-900 and slate-800.

This is @krishnaacharyaa @chinmaykunkikar your decision. If both of you seems this method to useful let me know I will push the code.

@krishnaacharyaa
Copy link
Owner

No the whole purpose gets defeated if you use the hex code, you need to directly use the tw classes that's the task, let us know if you find it challenging, I'll do it no issues ...

@SankalpHaritash21
Copy link
Contributor Author

SankalpHaritash21 commented Nov 30, 2023

const colors = require('tailwindcss/colors')

module.exports = {
theme: {
colors: {
dark: {
DEFAULT: colors.slate,
textfield: color.slate,
}
},
},
}

@krishnaacharyaa I tryed this approach also. but it I am not able to complete
If you can solve this issue you can. I failed to complete this issue. plz solve this issue I already taken more than 1 month on single issue. Now not to take more time.

and plz abhi muje unassign maat karna I also want to see the approach you are using.

@krishnaacharyaa
Copy link
Owner

@SankalpHaritash21 no worries, we got this, Thank you for the PR , I'll merge this PR don't worry, I'll create new PR and tag you there so that you can see the change ....

@SankalpHaritash21
Copy link
Contributor Author

@krishnaacharyaa can i take home page issue if u like

@chinmaykunkikar
Copy link
Collaborator

@SankalpHaritash21 don't worry. You came a long way. Kudos! :)
@krishnaacharyaa please merge this commit, I'll do these small changes in the skeleton PR.

Copy link
Owner

@krishnaacharyaa krishnaacharyaa left a comment

Choose a reason for hiding this comment

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

Merging it now :)

@krishnaacharyaa krishnaacharyaa merged commit 91567b1 into krishnaacharyaa:main Nov 30, 2023
3 checks passed
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.

Support Dark theme based on the web browser's theme
3 participants