Refactor navbar + reusable button components #13
Conversation
MelanieKent
changed the title
| @@ -36,6 +36,7 @@ jobs: | |||
| npm run lint | |||
|
|
|||
| - name: Build frontend | |||
| if: false | |||
There was a problem hiding this comment.
Skipping the frontend build step for now - the build is failing because there is a FlowLeaflets logo that is stored in the assets folder with gitignore. Not sure what's the best way around this?
There was a problem hiding this comment.
I think what we can do for the frontend is to put just the Flowleaflets logo from the root assets folder into the frontend one, and import from there instead. I don't think we'll need most of the other assets from the root folder for the ui so this should be ok.
| @@ -36,6 +36,7 @@ jobs: | |||
| npm run lint | |||
|
|
|||
| - name: Build frontend | |||
| if: false | |||
There was a problem hiding this comment.
I think what we can do for the frontend is to put just the Flowleaflets logo from the root assets folder into the frontend one, and import from there instead. I don't think we'll need most of the other assets from the root folder for the ui so this should be ok.
| @@ -0,0 +1,34 @@ | |||
| import Logo from "../../../../assets/flow-leaflets-logo.svg"; | |||
There was a problem hiding this comment.
If implementing the above suggestion, change this import to the frontend assets folder
| <p className="nav-button-text">History</p> | ||
| </NavLink> | ||
| </div> | ||
| <button className="logout-button" onClick={handleSignOut}> |
There was a problem hiding this comment.
Minor suggestion, but should we add some text to indicate logout button here?
There was a problem hiding this comment.
I think this is more of a question for the designers. It might be best to wait for their input before making changes from the Figma design. What do you think?
frontend/src/pages/Homepage.jsx
Outdated
| }; | ||
|
|
||
| const handleAddLogbook = () => { | ||
| navigate("/uploadPhotos") |
There was a problem hiding this comment.
This should redirect to /logcode once #12 has been merged in
| <CTASection /> | ||
| </div> | ||
| </NavContentWrapper> | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Another small suggestion, to keep things consistent with homepage, perhaps call this function MainContent
frontend/src/pages/Homepage.jsx
Outdated
There was a problem hiding this comment.
Another suggestion for consistency and for making it easier to implement tests is to move all the pages into its own folder with a __tests__ folder, similar to how login and sign_up folders are currently implemented. For the tests file inside each page, a simple test will do for now.
PR Information
dashboardcode intohomepagesince dashboard no longer exists in the Figma.<NavContentWrapper>and it should format everything out-of-the-box.If anything looks off, please let me know and I can fix it! 😄
Visuals
Recording.2024-11-26.235538.mp4
(Ignore the random duplicate button groups, just demonstrating how scrolling works). Have not made any significant changes to the layout of any pages