-
Notifications
You must be signed in to change notification settings - Fork 1
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
add sidebar/nav and routes Co-authored-by: Amr Ahmed #12
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @codepantherr
###What an amazing job, it's working as expected, the styling is like in the design provided and also you created functional components for other features in the project, good one!!
There is nothing else to say other than... is time to merge!!! 🎸 🕺 🎸 🕺
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## Hello @Amrhub
It's amazing how you implemented the dark them, taking care of all details like the logo in the navbar and the project is also responsive!!
Congratulations!!
Feel free to merge it! 🚀 🕺 🎸
src/components/sidebar/Sidebar.jsx
Outdated
<nav> | ||
<ul> | ||
<li className="nav-item"> | ||
<NavLink to="/" className="nav-link"> | ||
Doctors | ||
</NavLink> | ||
</li> | ||
<li className="nav-item"> | ||
<NavLink to="/book-appointment" className="nav-link"> | ||
Reserve | ||
</NavLink> | ||
</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This is an optional Change:
Everything is working as expected but I believe is a good idea to improve de UX to close the menu after the user selects an option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I will work on that now 🤩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @Amrhub
Your project is complete!!
There is nothing else to say other than... it's time to merge!!! 🚀 🟢
useEffect(() => { | ||
setTheme(localStorage.getItem('theme')); | ||
const localTheme = theme; | ||
if (localTheme === 'dark') { | ||
document.getElementById('theme-toggler').checked = true; | ||
} | ||
}, [theme]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great feature, congratulations!!
keep rocking!! 🎸 🕺
@AlexRS90 Thank you for the review ❤️ |
Changes introduced via this PR: 👇