Fix:Sidebar icons on minimized/closed position are not linked to rele… #264
Conversation
arkumari2000
changed the title
There was a problem hiding this comment.
Great job, @arkumari2000 for fixing the bug 👍. I've tested this on my local device and here's the test summary.
-
Code review: Done
-
Tested all possible outcomes.
Expected behaviour: reasonable sizes of sidebar is shown on closed/opened positions.
Actual behaviour: as expected
-
Environment:
OS: MacOS BigSur
Browser: Microsoft Edge
Thank you again for your contribution 🎉🎉🎉.
We only need one more approving review before this can be merged. cc @Rahulm2310 , can you please help with this?
mtreacy002
added
Category: Coding
src/sidebar/Sidebar.jsx
Outdated
| @@ -15,7 +15,7 @@ export default function Sidebar(){ | |||
| return ( | |||
| <div className={!isAuth ? "hidden" : ""}> | |||
There was a problem hiding this comment.
@arkumari2000 I don't think this is the right way to do conditional rendering. Anyone can change the toggle the display:none css property from the developer tools. Ideally, if isAuth is false, then the component should return null. So please don't use this hidden class as a way of conditional rendering a component.
cc: @mtreacy002
There was a problem hiding this comment.
I agree with @Rahulm2310 , @arkumari2000 . Perhaps the reason why you used hidden is because you didn't run the backend servers in parallel as you're supposed to and use this to by pass the isAuth. For this time, we can let it slide since the manual tests shows that it works when I run the backend servers. But for it to be merged to the upstream branch, can you please remove the hidden option as per @Rahulm2310 suggested? Thanks
There was a problem hiding this comment.
Yes sure, I will do the conditional rendering and not hidden property.
Rahulm2310
added
Status: Changes Requested
There was a problem hiding this comment.
@arkumari2000 , thank you for accommodating the requested changes. @Rahulm2310 , I think this is Ok now since the isAuth is no longer using conditional rendering (the rest of conditional rendering only related to the user's action on the sidebar after login is successful. So, I'm happy to approve this now. Let me know what you think 😉.
| onClick={() => setNotActive(!isNotActive)} | ||
| className="btn btn-custom" | ||
| > | ||
| <span className={isNotActive ? "" : "hidden"}>{barsIcon}</span> |
There was a problem hiding this comment.
@arkumari2000 Can you do the conditional rendering here for the menu icon as well
| ); | ||
| }; | ||
| ); | ||
| }else{ |
There was a problem hiding this comment.
You can use ternary operator instead of if-else. Like return isAuth? <component> : ""
| key="bottom" | ||
| placement="bottom" | ||
| overlay={ | ||
| <Tooltip id={`tooltip-$'bottom'`}> |
There was a problem hiding this comment.
Similar to this, there should be tooltips for all menu links
|
Any updates @arkumari2000 ? |
|
Closing the PR due to inactivity. |
Description
Atm the Sidebar icons are now linked to the relevant pages.
Fixes #260
Type of Change:
Code/Quality Assurance Only
How Has This Been Tested?
I didn't set up the BIT backend and tested it without logging in by removing hidden from the code, but the code I pushed is perfectly fine which shows the sidebar only to the logged-in person.
And as I didn't log in, the video will contain the login page when I click on links but please focus on the bottom left corner. This will direct the logged-in person to the perfect link.
123808994-da4a3d00-d90e-11eb-95d0-21f19328cfc8.mov
Checklist:
Code/Quality Assurance Only