Skip to content

Conversation

@manish591
Copy link
Contributor

@manish591 manish591 commented Dec 22, 2022

fixes #272

NOTE: I have taken a pull from Vinayak's PR #274 that is already been reviewed.

Visual Reference

Screenshot (38)

akshay1502 and others added 16 commits August 22, 2022 22:42
* Changes in navbar UI and content

* Change in navbar UI

* Change in navbar UI as per new design

* Add login url from constants

* Added changes to fetch and display data for signed-in users

* Fixed display data for signed-out users

* Added mobile UI change for currency exchange page

* Added changes to use Link component for anchor tags

* Remove commented code

* css code refactor changes

* Add and load paths from constants file

* Remove !important property from navbar css

* Refactor css code

* Add UI changes in navbar

* Add default avatar for user profile picture

* Add UI changes in navbar

* Add UI changes in navbar

* Add profile link on greet message & error handling in fetch user

* Fix profile url

* Added .env.development file (RealDevSquad#245)

Co-authored-by: user.name: "Rohan Raj Gupta <user.email: "rajrohan1914@gmail.com>

* Improvement in code and changes to use cloudinary images

* Add key prop

* Replace useEffect by useLayoutEffect for login button

* Improvement in navbar code

* Update ecmaVersion in ESLint config

* Revert useLayoutEffect changes for login button

* Fix login button UI

Co-authored-by: Shubham <yadav105shubham@gmail.com>
Co-authored-by: Rohan Raj Gupta <78433013+rohan09-raj@users.noreply.github.com>
Co-authored-by: user.name: "Rohan Raj Gupta <user.email: "rajrohan1914@gmail.com>
* Changes in navbar UI and content

* Change in navbar UI

* Change in navbar UI as per new design

* Add login url from constants

* Added changes to fetch and display data for signed-in users

* Fixed display data for signed-out users

* Added mobile UI change for currency exchange page

* Added changes to use Link component for anchor tags

* Remove commented code

* css code refactor changes

* Add and load paths from constants file

* Remove !important property from navbar css

* Refactor css code

* Add UI changes in navbar

* Add default avatar for user profile picture

* Add UI changes in navbar

* Add UI changes in navbar

* Add profile link on greet message & error handling in fetch user

* Fix profile url

* Added .env.development file (RealDevSquad#245)

Co-authored-by: user.name: "Rohan Raj Gupta <user.email: "rajrohan1914@gmail.com>

* Improvement in code and changes to use cloudinary images

* Add key prop

* Replace useEffect by useLayoutEffect for login button

* Improvement in navbar code

* Update ecmaVersion in ESLint config

* Revert useLayoutEffect changes for login button

* Fix login button UI

Co-authored-by: Shubham <yadav105shubham@gmail.com>
Co-authored-by: Rohan Raj Gupta <78433013+rohan09-raj@users.noreply.github.com>
Co-authored-by: user.name: "Rohan Raj Gupta <user.email: "rajrohan1914@gmail.com>
@vercel
Copy link

vercel bot commented Dec 22, 2022

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

Name Status Preview Comments Updated
website-crypto ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 18, 2023 at 5:45PM (UTC)

@manish591 manish591 marked this pull request as draft December 22, 2022 16:55
@manish591 manish591 marked this pull request as ready for review January 11, 2023 19:08
@manish591 manish591 marked this pull request as draft January 11, 2023 19:11
@manish591 manish591 marked this pull request as ready for review January 12, 2023 14:51
@heyrandhir
Copy link

@manish591 can't find the test in the PR. Are the test already merged 🤔.

@manish591
Copy link
Contributor Author

@manish591 can't find the test in the PR. Are the test already merged 🤔.

No, the tests are not merged yet. Will write them now

@samyakshah3008 samyakshah3008 self-requested a review February 16, 2023 04:20
samyakshah3008
samyakshah3008 previously approved these changes Feb 18, 2023
Copy link

@samyakshah3008 samyakshah3008 left a comment

Choose a reason for hiding this comment

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

Solid work Manish!

Here are few pointers which you can refactor:

  1. Instead of using px, use rem.
  2. Don't use !important
  3. Avoid white spaces

Added few single line comments, rest is all good for approval.

Copy link

@heyrandhir heyrandhir left a comment

Choose a reason for hiding this comment

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

Great work @manish591! .Would recommend you creating small and incremental PRs in the future. Thank you!

@manish591
Copy link
Contributor Author

Great work @manish591! .Would recommend you creating small and incremental PRs in the future. Thank you!

ok

display: flex;
justify-content: space-between;
padding: 1.6rem;
background-color: var(---bg-blue-300);
Copy link

Choose a reason for hiding this comment

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

There appears to be a CSS variable name typo in the sidebar mobile toggle styling. The property background-color: var(---bg-blue-300); has an extra dash - it should be background-color: var(--bg-blue-300); (with two dashes instead of three). This will ensure the mobile toggle background color renders correctly.

Suggested change
background-color: var(---bg-blue-300);
background-color: var(--bg-blue-300);

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

@@ -0,0 +1,5 @@
export default {
ICON_PATH_ARROW: '/assets/rrow.svg',
Copy link

Choose a reason for hiding this comment

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

There appears to be a typo in the icon path for the arrow. The current path is '/assets/rrow.svg' but should likely be '/assets/arrow.svg' or '/assets/MenuArrow.svg' (which exists in the public assets directory based on the PR). This could cause the arrow icon to not load properly in the UI.

Suggested change
ICON_PATH_ARROW: '/assets/rrow.svg',
ICON_PATH_ARROW: '/assets/MenuArrow.svg',

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

'June',
'July',
'August',
'Sepetember',
Copy link

Choose a reason for hiding this comment

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

There's a typo in the month name - 'Sepetember' should be spelled 'September'. This appears in the months list constant that's used for the transaction dropdown.

Suggested change
'Sepetember',
'September',

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

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.

[UI Revamp] New dashboard UI

7 participants