Skip to content
This repository has been archived by the owner on Aug 7, 2024. It is now read-only.

Implemented the Dark/Light-Mode feature #1852

Merged
merged 3 commits into from
Oct 8, 2022
Merged

Implemented the Dark/Light-Mode feature #1852

merged 3 commits into from
Oct 8, 2022

Conversation

Vedant-Manjrekar
Copy link
Contributor

@Vedant-Manjrekar Vedant-Manjrekar commented Oct 1, 2022 •
edited by schmelto
Loading

Fixes Issue #1032

resolve #1032

Changes proposed

Check List (Check all the applicable boxes)

  • My code follows the code style of this project.
  • My change requires changes to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.
  • This PR does not contain plagiarized content.
  • The title of my pull request is a short description of the requested changes.

Screenshots

My code passed all the test

Screenshot 2022-10-01 at 6 33 37 PM

SS of the feature Implemented

Screenshot 2022-10-01 at 6 27 21 PM

Screenshot 2022-10-01 at 6 27 34 PM

Screenshot 2022-10-01 at 6 28 15 PM

4 PM.png…]()

Screenshot 2022-10-01 at 6 28 03 PM

@github-actions
Copy link
Contributor

github-actions bot commented Oct 1, 2022

Welcome @Vedant-Manjrekar! Thank you so much for your first pull request!

@github-actions github-actions bot added large Pull request with more than 30 changed lines waiting-for-reviewers labels Oct 1, 2022
github-actions[bot]
github-actions bot reviewed Oct 1, 2022
View reviewed changes
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

It's great having you contribute to this project

Welcome to the community 🤓

If you would like to continue contributing to open source and would like to do it with an awesome inclusive community, you should join our Discord chat and our GitHub Organisation - we help and encourage each other to contribute to open source little and often 🤓 . Any questions let us know.

gptkrsh
gptkrsh suggested changes Oct 1, 2022
View reviewed changes
Copy link
Contributor

@gptkrsh gptkrsh left a comment

Choose a reason for hiding this comment

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

Can the navbar not be dark? Also I don't think if adding so many dependencies for a dark mode is a good idea

@anirudhp26
Copy link
Contributor

@Vedant-Manjrekar Maybe we can use theme like one of Youtube
Different shades of grey xD

EmmaDawsonDev
EmmaDawsonDev suggested changes Oct 1, 2022
View reviewed changes
Copy link
Member

@EmmaDawsonDev EmmaDawsonDev left a comment

Choose a reason for hiding this comment

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

Please try to do this with the libraries already installed

package.json Outdated
@@ -4,6 +4,11 @@
"private": false,
"homepage": "https://linkfree.eddiehub.org",
"dependencies": {
"@emotion/react": "^11.10.4",
Copy link
Member

Choose a reason for hiding this comment

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

We are using prime react for the current version of linkfree. Please try to look at their implementation of dark mode instead of installing lots of new packages

package.json Outdated
"@emotion/styled": "^11.10.4",
"@mui/icons-material": "^5.10.6",
"@mui/material": "^5.10.7",
"@reduxjs/toolkit": "^1.8.5",
Copy link
Member

Choose a reason for hiding this comment

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

I think redux is too much just for handling dark mode. If we need any global state then context should be considered first if it can’t be done with just prime react components

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But to add the theme changes across multiple pages simultaneously a global state is required, hence i think there is a need to use redux.

Copy link
Member

Choose a reason for hiding this comment

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

Or context or just local storage. I believe Redux is too much when we’re not handling any other global state.

@Vedant-Manjrekar
Copy link
Contributor Author

New commit includes:

  1. Removal extra dependencies which were previously added as well as refactored the code, hence instead of redux, useContext hook is now used to implement the light / dark mode feature as suggested by @EmmaDawsonDev
  2. Improved UI of Dark-mode as suggested by @anirudhp26

SS :
Screenshot 2022-10-02 at 5 31 58 PM
Screenshot 2022-10-02 at 5 32 14 PM
Screenshot 2022-10-02 at 5 32 36 PM
Screenshot 2022-10-02 at 5 32 58 PM
Screenshot 2022-10-02 at 5 33 16 PM
Screenshot 2022-10-02 at 5 33 26 PM

@Vedant-Manjrekar Vedant-Manjrekar requested review from EmmaDawsonDev and gptkrsh and removed request for EmmaDawsonDev October 2, 2022 12:35
@anirudhp26
Copy link
Contributor

@Vedant-Manjrekar looks great ✨

@Vedant-Manjrekar
Copy link
Contributor Author

@Vedant-Manjrekar looks great ✨

@anirudhp26 Thanks man 😃

@Vedant-Manjrekar Vedant-Manjrekar requested review from EmmaDawsonDev and removed request for gptkrsh October 4, 2022 07:41
@Vedant-Manjrekar
Copy link
Contributor Author

@EmmaDawsonDev @schmelto I have submitted a PR regarding the dark/light theme issue can you take a look at it ?

eddiejaoude
eddiejaoude reviewed Oct 8, 2022
View reviewed changes
src/Components/Search/Users.js Outdated

const theme = {
backgroundColor: `${darkTheme ? '#333333' : '#dee2e6'}`,
// border: `${darkTheme ? 'none' : '1px solid black'}`,
Copy link
Member

Choose a reason for hiding this comment

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

Is this commented out code required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry my bad.

eddiejaoude
eddiejaoude approved these changes Oct 8, 2022
View reviewed changes
Copy link
Member

@eddiejaoude eddiejaoude left a comment

Choose a reason for hiding this comment

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

Looks great 👍

@EmmaDawsonDev
Copy link
Member

@Vedant-Manjrekar Can you please solve the merge conflicts

@Vedant-Manjrekar
Copy link
Contributor Author

@Vedant-Manjrekar Can you please solve the merge conflicts

Done 👍🏽

@EmmaDawsonDev EmmaDawsonDev merged commit 7e718cc into EddieHubCommunity:main Oct 8, 2022
E-fais
E-fais reviewed Oct 9, 2022
View reviewed changes
Copy link
Member

@E-fais E-fais left a comment

Choose a reason for hiding this comment

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

LGTM

@Vedant-Manjrekar Vedant-Manjrekar deleted the vedant_darkMode_feature branch October 9, 2022 09:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@github-actions GitHub Actions GitHub Actions left review comments

@E-fais E-fais E-fais left review comments

@gptkrsh gptkrsh gptkrsh requested changes

@eddiejaoude eddiejaoude eddiejaoude approved these changes

@EmmaDawsonDev EmmaDawsonDev EmmaDawsonDev approved these changes

Assignees
No one assigned
Labels
🕹 aspect: interface undefined ⭐ goal: addition undefined large Pull request with more than 30 changed lines waiting-for-reviewers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEATURE] Light/Dark Mode
7 participants
@Vedant-Manjrekar @anirudhp26 @EmmaDawsonDev @eddiejaoude @gptkrsh @E-fais @Panquesito7