Dark mode #242

shubham-y · 2021-10-21T12:53:08Z

Closes #239

components/Dark-Theme/useDarkMode.js

shubham-y · 2021-11-05T13:35:46Z

After changes

updated-dark-mode.mp4

prakashchoudhary07

Wow, this is Great PR 🎉 🎉

…rypto into dark-mode

shubham-y · 2022-04-16T07:44:38Z

Made changes to use the cookie as a feature flag

dark-mode-update_m07JVukg.mp4

rohan09-raj

@shubham-y Have you removed the implementation of Icons ??

components/custom-button/custom-button.style.js

rohan09-raj

Looks good to me , approving the PR

Neha

Code review is in WIP

Neha · 2022-06-23T18:17:47Z

components/Auction/new.module.css

-  border: 0.5px solid #ccc;
+  border: 0.5px solid var(--colorAuctionInput);
+  background-color: var(--colorBackgroundDark);
+  color: var(--colorWhite);
  border-radius: 5px;
  padding: 10px;
  padding-left: 10px;


repetitive code. Please remove padding-left

Neha · 2022-06-23T18:19:19Z

components/Auction/new.module.css

+.inputBox:disabled{
+  background-color: var(--colorBackgroundLight);
+}
+
 .submitBtn {
  cursor: pointer;
  background-color: rgb(27, 199, 27);
  color: white;


Consistency is missing in colour value declarations: either use rgb or name of color of code of color.
Go with RGBA as it is easy to control the alpha value.

Neha · 2022-06-23T18:19:56Z

components/Dark-Theme/Themes.js

@@ -0,0 +1,48 @@
+export const lightTheme = {
+  colorBackgroundLight: '#f4f4f4',


Light/medium/Dark has same value. Why to make 2 variables for the same value?

Neha · 2022-06-23T18:21:46Z

components/Dark-Theme/Themes.js

+  colorExchangeBackground: 'aliceblue',
+  colorBodyBackground: '#e9ebff',
+  colorText: '#363636',
+  colorTransactionBorder: '#f0e2e7',


From there onwards, variable names are tied with the component. Make generic names for the variables.

Neha · 2022-06-23T18:22:09Z

components/Dark-Theme/Themes.js

+  colorButtonBorder: 'white',
+  colorFilterHover: '#bdc3c7',
+  colorAuctionBackground: 'rgb(245, 235, 235)',
+  colorWhite: 'black',


name is colorWhite but the value is black. Please correct this

Neha · 2022-06-23T18:51:31Z

components/NavBar/index.js

@@ -12,6 +14,7 @@ const NavBar = () => {
  const RDS_LOGO = '/assets/Real-Dev-Squad1x.png';
  const GITHUB_LOGO = '/assets/github.png';
  const DEFAULT_AVATAR = '/assets/default_avatar.jpg';
+  const [theme, themeData, themeToggler] = useDarkModeContext();


From there themeData is not getting used anywhere

Neha · 2022-06-23T18:52:04Z

components/NavBar/index.js

@@ -12,6 +14,7 @@ const NavBar = () => {
  const RDS_LOGO = '/assets/Real-Dev-Squad1x.png';


Move all the assests URL to constant

Neha · 2022-06-23T20:32:46Z

components/dark-theme-icon/index.js

+
+function DarkThemeIcon({ theme, themeToggleHandler }) {
+  return (
+    <div


This should be button and not DIV

Neha · 2022-06-23T20:33:05Z

components/dark-theme-icon/index.js

+      className={classNames.container}
+    >
+      {theme === 'light' ? (
+        <img src="/assets/moon.png" alt="moon" />


this should come from constant also alt tag is wrong.

Neha · 2022-06-23T20:33:26Z

components/dark-theme-icon/index.js

+      {theme === 'light' ? (
+        <img src="/assets/moon.png" alt="moon" />
+      ) : (
+        <img src="/assets/sun.png" alt="sun" />


this should come from constant

Neha · 2022-06-23T20:42:26Z

components/Dark-Theme/useDarkMode.js

+    }
+    setMountedComponent(true);
+  }, []);
+  return [theme, themeData, themeToggler, mountedComponent];


We should return the Object from here. This will help us to destruct values based on keys. Then we can avoid getting all values (even if we don't need them) and we can get them in any order

Neha · 2022-06-23T21:14:53Z

components/Dark-Theme/globalStyles.js

+  :root{
+  ${({ theme }) => {
+    let style = ``;
+    for (const variableName in theme) {


The name should property rather then variableName

Neha · 2022-06-23T21:16:15Z

components/filter/index.js

@@ -2,8 +2,10 @@ import styles from './filter.module.css';
 import Image from 'next/image';
 import { useState, useRef } from 'react';


Always have React import on the top.
Then the rest of the importa

Neha · 2022-06-23T21:17:53Z

components/filter/index.js

-          src="/assets/filter-icon.svg"
+          src={
+            theme === 'light'
+              ? '/assets/filter-icon.svg'


Move the image src and the content to constant file. This logic could be a component as reusable. This is also getting used in Nav components also.

Neha · 2022-06-23T21:20:51Z

components/filter/index.js

+            theme === 'light'
+              ? '/assets/filter-icon.svg'
+              : '/assets/filter-icon-dark.svg'
+          }
          alt="Filter icon"
          width={20}


width and height should come from constant. As well, the UL should come dynamic. create an Object with the list of options required... coop over it to create a List and then bind the event on it. Avoid doing any hardcoding of the content

Neha · 2022-06-23T21:32:57Z

components/NavBar/navbar.module.css

@@ -182,6 +187,10 @@
    float: none;


We should not be using float at all.

Neha · 2022-06-23T21:35:09Z

components/stock-card/index.js

@@ -77,13 +77,13 @@ export const Card = ({ stock, operationType }) => {
            text-align: center;
            width: 100%;
            padding: 1rem;
-            background-color: #ffffff;
+            background-color: var(--colorBackgroundMedium);
          }

          .stock-card-product-name {


Class name format is not consistent. Use camelCase

Neha · 2022-06-23T21:36:26Z

pages/auction/index.js

@@ -5,23 +5,44 @@ import { Footer } from '@components/footer';
 import Head from 'next/head';
 import HandleAuctions from '@components/Auction';
 import styles from '@components/Auction/Auction.module.css';
+import GlobalStyles from '@components/Dark-Theme/globalStyles';
+import { ThemeProvider } from 'styled-components';


Move the 3rd party imports on the top of the component

Neha · 2022-06-23T21:38:01Z

pages/auction/index.js

+    <ThemeProvider theme={themeData}>
+      <>
+        <GlobalStyles />
+        <div>


DIV should start from the Navbar rather than before HEAD

Neha · 2022-06-23T21:41:46Z

pages/auction/new.js

+            />
+          </Head>
+          <NavBar personData={personData} />
+          <Link href="/auction">


Either use Link or a. Both should not be used together. Use Link for routing and for 3rd party use a tag

Neha · 2022-06-23T21:42:00Z

pages/auction/new.js

+          <NavBar personData={personData} />
+          <Link href="/auction">
+            <a>
+              <h2 className={styles.auctionPageNavigation}>Go Back</h2>


Move the content to constant file

Neha · 2022-06-23T21:42:42Z

pages/auction/new.js

+          <CreateNewAuction />
+          <Footer />
+        </div>
+      </>


If you are wrapping with DIV tag then you don't need Fragment tags

Neha · 2022-06-23T21:48:15Z

pages/auction/new.js

@@ -5,23 +5,42 @@ import { Footer } from '@components/footer';
 import Head from 'next/head';
 import CreateNewAuction from '@components/Auction/createNewAuction';
 import styles from '@components/Auction/Auction.module.css';
+import GlobalStyles from '@components/Dark-Theme/globalStyles';
+import { ThemeProvider } from 'styled-components';


Move this on the top

Neha · 2022-06-23T21:53:35Z

pages/currency-exchange/index.js

+    <ThemeProvider theme={themeData}>
+      <>
+        <GlobalStyles />
+        <div>


Div should be moved after Head closing. As well as the fragment is not required. Make Head part as a reusable component to re-use

Neha · 2022-06-23T22:01:35Z

pages/auction/index.js

+          <Link href="/auction/new">
+            <div className={styles.newAuctionBtnContainer}>
+              <button className={styles.newAuctionBtn}>
+                Create New Auction


Move this to constant

MehulKChaudhari · 2022-07-07T09:36:56Z

Hey @shubham-y, Could you make suggested changes or are you working on something else?

MehulKChaudhari · 2022-07-15T13:48:34Z

As we are changing the UI of the crypto site. We don't need these changes at the moment. So I am marking this as stale.

shubham-y added 4 commits October 21, 2021 18:17

Add css variables for dark mode

2090467

Add icons for dark mode

cc85be4

Add Dark-Theme and dark-theme-icon components

7682c8c

Changes in pages and components for dark theme

8802689

shubham-y self-assigned this Oct 21, 2021

shubham-y added feature task A big ticket item that needs to come up as a feature frontend labels Oct 21, 2021

shubham-y requested a review from RohitKuwar October 21, 2021 12:56

shubham-y added 3 commits October 21, 2021 18:29

Changes in dark mode context

92c4317

Fix dark mode for mobile version

ca990d8

Add cookie for dark theme

9fd5006

Pavangbhat suggested changes Nov 1, 2021

View reviewed changes

components/Dark-Theme/useDarkMode.js Outdated Show resolved Hide resolved

ghost suggested changes Nov 3, 2021

View reviewed changes

components/Dark-Theme/useDarkMode.js Outdated Show resolved Hide resolved

components/Dark-Theme/useDarkMode.js Outdated Show resolved Hide resolved

shubham-y added 3 commits November 5, 2021 16:24

Added cookie functions in utils

85c07f8

Rename css variables

6224ca0

Changes in transaction chart for dark mode

1225995

shubham-y marked this pull request as ready for review November 5, 2021 13:36

shubham-y requested a review from prakashchoudhary07 November 5, 2021 13:38

prakashchoudhary07 previously approved these changes Nov 9, 2021

View reviewed changes

Merge branch 'develop' of https://github.com/Real-Dev-Squad/website-c…

fa519c1

…rypto into dark-mode

shubham-y dismissed prakashchoudhary07’s stale review via fa519c1 February 25, 2022 19:35

shubham-y added 3 commits February 26, 2022 02:10

Changes in navbar link for dark mode

99a20ea

Add default theme for dark mode

b16b137

Fix toggle icon for mobile view

f34c9d2

rohan09-raj reviewed Apr 17, 2022

View reviewed changes

components/custom-button/custom-button.style.js Outdated Show resolved Hide resolved

Remove unwanted code

daf5b90

rohan09-raj previously approved these changes Apr 19, 2022

View reviewed changes

Refactor globalStyle file

142500f

Fix variable name

fbb6599

shubham-y dismissed rohan09-raj’s stale review via fbb6599 June 23, 2022 17:59

Neha suggested changes Jun 23, 2022

View reviewed changes

Neha reviewed Jun 23, 2022

View reviewed changes

MehulKChaudhari added the Stale Not needed at the moment. label Jul 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dark mode #242

Dark mode #242

shubham-y commented Oct 21, 2021

shubham-y commented Nov 5, 2021

prakashchoudhary07 left a comment

shubham-y commented Apr 16, 2022

rohan09-raj left a comment

rohan09-raj left a comment

Neha left a comment

Neha Jun 23, 2022

Neha Jun 23, 2022

Neha Jun 23, 2022

Neha Jun 23, 2022

Neha Jun 23, 2022

Neha Jun 23, 2022

Neha Jun 23, 2022

Neha Jun 23, 2022

Neha Jun 23, 2022

Neha Jun 23, 2022

Neha Jun 23, 2022

Neha Jun 23, 2022

Neha Jun 23, 2022

Neha Jun 23, 2022

Neha Jun 23, 2022

Neha Jun 23, 2022

Neha Jun 23, 2022

Neha Jun 23, 2022

Neha Jun 23, 2022

Neha Jun 23, 2022

Neha Jun 23, 2022

Neha Jun 23, 2022

Neha Jun 23, 2022

Neha Jun 23, 2022

Neha Jun 23, 2022

MehulKChaudhari commented Jul 7, 2022 •

edited

Loading

MehulKChaudhari commented Jul 15, 2022

		@@ -0,0 +1,48 @@
		export const lightTheme = {
		colorBackgroundLight: '#f4f4f4',

		@@ -12,6 +14,7 @@ const NavBar = () => {
		const RDS_LOGO = '/assets/Real-Dev-Squad1x.png';

		@@ -2,8 +2,10 @@ import styles from './filter.module.css';
		import Image from 'next/image';
		import { useState, useRef } from 'react';

Dark mode #242

Are you sure you want to change the base?

Dark mode #242

Conversation

shubham-y commented Oct 21, 2021

shubham-y commented Nov 5, 2021

prakashchoudhary07 left a comment

Choose a reason for hiding this comment

shubham-y commented Apr 16, 2022

rohan09-raj left a comment

Choose a reason for hiding this comment

rohan09-raj left a comment

Choose a reason for hiding this comment

Neha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MehulKChaudhari commented Jul 7, 2022 • edited Loading

MehulKChaudhari commented Jul 15, 2022

MehulKChaudhari commented Jul 7, 2022 •

edited

Loading